generated from TrueCloudLab/basic
[#1] linters: add logs linter #2
No reviewers
Labels
No labels
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/linters#2
Loading…
Reference in a new issue
No description provided.
Delete branch "achuprov/linters:master"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #1
@ -0,0 +2,4 @@
PLUGIN_SOURCE?=main.go
test:
@go test -v ./...
Be default
go test
caches test result. This may cause flaky tests to be cached.@go test ./... -count=1
will disable caching.@ -0,0 +9,4 @@
@go build -buildmode=plugin -o $(OUT_DIR)/external_linters.so $(PLUGIN_SOURCE)
lint:
@golangci-lint run
Please add
.golangci.yml
file with linter config like here: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/.golangci.ymlThe empty repository template already has a set of minimally necessary linters, therefore .golangci.yml is not present in the PR
@ -0,0 +1,88 @@
package noliteral
I think we need to rename the package name or directory.
https://go.dev/doc/effective_go#package-names
@ -0,0 +20,4 @@
var (
methodsToSearchOnce = &sync.Once{}
methodsToSearch = []string{"Debug", "Info", "Warn", "Error"}
customLogs = "reportFlushError, reportError"
I think we can merge
customLogs
tomethodsToSearch
and removemethodsToSearchOnce
.@ -0,0 +23,4 @@
customLogs = "reportFlushError, reportError"
)
func run(pass *analysis.Pass) (interface{}, error) {
Looks to complex: there are 5 code levels. Please run
make lint
after adding.golangci.yml
configuration file.@ -0,0 +11,4 @@
"golang.org/x/tools/go/analysis"
)
func TestAnalyzerA_n(t *testing.T) {
What does mean
A
,B
,C
,D
? Function names should be more meaningful.@ -0,0 +74,4 @@
}
}
func TestAnalyzerB_n(t *testing.T) {
Copy-paste from
TestAnalyzerA_n
. Unit tests can also use methods to not to duplicate same code.@ -0,0 +1,36 @@
package src_test
func (c *cfg) f1_n() {
c.log.Info("logs.MSG") //unacceptable
I think that the number of files can be reduced to 2: one for positive cases, one for negative ones. In each file there will be calls
Info
,Debug
,Warn
,Error
methods.Can we check that log arguments are actually constants coming from a configurable package up to renames? Rather than just checking if it's something of the form
logs.M
, whetherlogs
is a struct or something else, which paves the way for tricking the linter as the unit tests do.(unless that's not part of the plan @dstepanov-yadro)
You're right. The plan is to make the linter smarter in the following iterations: add a check of the source file with constants, check a specific type of logger, make the test configurable, etc.
f3e71c0714
to578611f4f4
All remarks from @dstepanov-yadro have been corrected in
be56554a8f
@ -0,0 +11,4 @@
"golang.org/x/tools/go/analysis"
)
func TestAnalyzer_negaive(t *testing.T) {
typo: negative
@ -0,0 +42,4 @@
}
func TestAnalyzer_positive(t *testing.T) {
Please remove empty line after method definition
be56554a8f
tobc08e15622
@ -0,0 +1,18 @@
OUT_DIR?=./.bin
Nitpick: whitespace aroung
?=
is insignificant https://www.gnu.org/software/make/manual/html_node/Setting.htmlThis is the style we use in our code https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/Makefile#L4 , you may find it more readable.
fixed
@ -0,0 +8,4 @@
## Available linters
| Name | Description |
What editor do you use? I believe VSCode can align tables if you press
Tab
inside the field.fixed
@ -0,0 +25,4 @@
## Usage
Add to .golagci.yml
typo
.golaNgci.yml
fixed
@ -0,0 +15,4 @@
Run: run,
}
var methodsToSearch = []string{"Debug", "Info", "Warn", "Error", "reportFlushError", "reportError"}
reportFlushError
is not some common name. Can we move it to the configuration (maybe later)?@ -0,0 +36,4 @@
pass.Report(analysis.Diagnostic{
Pos: expr.Pos(),
End: 0,
Is
0
here intended?fixed
@ -0,0 +50,4 @@
}
func isLogDot(expr ast.Expr) (bool, string) {
sel, ok := expr.(*ast.SelectorExpr)
Objects from the imported package like
ast.SelectorExpr
areast.SelectorExpr
too, sois catched now. It is not a problem, but what about a test for this?
@ -0,0 +65,4 @@
func isIdent(expr ast.Expr, ident string) bool {
id, ok := expr.(*ast.Ident)
if !ok {
So 2 ifs can be replaced with
return ok && id.Name == ident
?You actually have already done this in
isStringValue
function.fixed
@ -0,0 +14,4 @@
return []*analysis.Analyzer{noliteral.LogsAnalyzer}
}
/*
Why is it commented?
bc08e15622
to719973b3fa
@fyrchik, thank you for your review. The main comments have been fixed in 719973b3fa.
Regarding the configuration: The 1.5.3 release of golangci-lint does not support passing the configuration from golangci.yml now. However, the next version is expected to support this feature. Therefore, this interface is considered unstable for now, and the 'New' function has been commented out.
I am already working on a workaround that would allow us to read configurations. I am going to implement flexible solution to be ready for stable version.
719973b3fa
to53d601603d
dstepanov-yadro referenced this pull request2023-07-31 09:18:04 +00:00