[#1] linters: add logs linter #2

Merged
dstepanov-yadro merged 1 commit from achuprov/linters:master into master 2023-07-26 21:08:06 +00:00
Member

Closes #1

Closes #1
achuprov added 1 commit 2023-07-18 08:19:35 +00:00
achuprov requested review from storage-core-committers 2023-07-18 09:28:15 +00:00
achuprov requested review from storage-core-developers 2023-07-18 09:28:15 +00:00
dstepanov-yadro requested changes 2023-07-18 10:49:10 +00:00
Makefile Outdated
@ -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.

Be default `go test` caches test result. This may cause flaky tests to be cached. `@go test ./... -count=1` will disable caching.
dstepanov-yadro marked this conversation as resolved
@ -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.yml

Please add `.golangci.yml` file with linter config like here: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/.golangci.yml
Author
Member

The empty repository template already has a set of minimally necessary linters, therefore .golangci.yml is not present in the PR

The empty repository template already has a set of minimally necessary linters, therefore .golangci.yml is not present in the PR
dstepanov-yadro marked this conversation as resolved
@ -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

Another convention is that the package name is the base name of its source directory; the package in src/encoding/base64 is imported as "encoding/base64" but has name base64, not encoding_base64 and not encodingBase64.
I think we need to rename the package name or directory. https://go.dev/doc/effective_go#package-names ``` Another convention is that the package name is the base name of its source directory; the package in src/encoding/base64 is imported as "encoding/base64" but has name base64, not encoding_base64 and not encodingBase64. ```
dstepanov-yadro marked this conversation as resolved
@ -0,0 +20,4 @@
var (
methodsToSearchOnce = &sync.Once{}
methodsToSearch = []string{"Debug", "Info", "Warn", "Error"}
customLogs = "reportFlushError, reportError"

I think we can merge customLogs to methodsToSearch and remove methodsToSearchOnce.

I think we can merge `customLogs` to `methodsToSearch` and remove `methodsToSearchOnce`.
dstepanov-yadro marked this conversation as resolved
@ -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.

Looks to complex: there are 5 code levels. Please run `make lint` after adding `.golangci.yml` configuration file.
dstepanov-yadro marked this conversation as resolved
@ -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.

What does mean `A`, `B`, `C`, `D`? Function names should be more meaningful.
dstepanov-yadro marked this conversation as resolved
@ -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.

Copy-paste from `TestAnalyzerA_n`. Unit tests can also use methods to not to duplicate same code.
dstepanov-yadro marked this conversation as resolved
@ -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.

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.
dstepanov-yadro marked this conversation as resolved
Member

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, whether logs 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)

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`, whether `logs` 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)

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, whether logs 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.

> 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`, whether `logs` 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.
achuprov force-pushed master from f3e71c0714 to 578611f4f4 2023-07-18 14:36:14 +00:00 Compare
Author
Member

All remarks from @dstepanov-yadro have been corrected in be56554a8f

All remarks from @dstepanov-yadro have been corrected in https://git.frostfs.info/TrueCloudLab/linters/commit/be56554a8fa7792c4d490592e5f7767896b91dfd
dstepanov-yadro requested changes 2023-07-19 06:53:07 +00:00
@ -0,0 +11,4 @@
"golang.org/x/tools/go/analysis"
)
func TestAnalyzer_negaive(t *testing.T) {

typo: negative

typo: nega**t**ive
dstepanov-yadro marked this conversation as resolved
@ -0,0 +42,4 @@
}
func TestAnalyzer_positive(t *testing.T) {

Please remove empty line after method definition

Please remove empty line after method definition
dstepanov-yadro marked this conversation as resolved
achuprov force-pushed master from be56554a8f to bc08e15622 2023-07-19 07:34:12 +00:00 Compare
achuprov requested review from dstepanov-yadro 2023-07-19 07:35:46 +00:00
dstepanov-yadro approved these changes 2023-07-19 08:17:15 +00:00
fyrchik reviewed 2023-07-21 09:06:02 +00:00
Makefile Outdated
@ -0,0 +1,18 @@
OUT_DIR?=./.bin
Owner

Nitpick: whitespace aroung ?= is insignificant https://www.gnu.org/software/make/manual/html_node/Setting.html
This 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.

Nitpick: whitespace aroung `?=` is insignificant https://www.gnu.org/software/make/manual/html_node/Setting.html This 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.
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
README.md Outdated
@ -0,0 +8,4 @@
## Available linters
| Name | Description |
Owner

What editor do you use? I believe VSCode can align tables if you press Tab inside the field.

What editor do you use? I believe VSCode can align tables if you press `Tab` inside the field.
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
README.md Outdated
@ -0,0 +25,4 @@
## Usage
Add to .golagci.yml
Owner

typo .golaNgci.yml

typo `.golaNgci.yml`
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
@ -0,0 +15,4 @@
Run: run,
}
var methodsToSearch = []string{"Debug", "Info", "Warn", "Error", "reportFlushError", "reportError"}
Owner

reportFlushError is not some common name. Can we move it to the configuration (maybe later)?

`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,
Owner

Is 0 here intended?

Is `0` here intended?
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
@ -0,0 +50,4 @@
}
func isLogDot(expr ast.Expr) (bool, string) {
sel, ok := expr.(*ast.SelectorExpr)
Owner

Objects from the imported package like ast.SelectorExpr are ast.SelectorExpr too, so

import "log"
log.Info(..)

is catched now. It is not a problem, but what about a test for this?

Objects from the imported package like `ast.SelectorExpr` are `ast.SelectorExpr` too, so ``` import "log" log.Info(..) ``` is 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 {
Owner

So 2 ifs can be replaced with return ok && id.Name == ident?
You actually have already done this in isStringValue function.

So 2 ifs can be replaced with `return ok && id.Name == ident`? You actually have already done this in `isStringValue` function.
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
main.go Outdated
@ -0,0 +14,4 @@
return []*analysis.Analyzer{noliteral.LogsAnalyzer}
}
/*
Owner

Why is it commented?

Why is it commented?
achuprov force-pushed master from bc08e15622 to 719973b3fa 2023-07-23 14:03:25 +00:00 Compare
Author
Member

@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.

@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.
aarifullin approved these changes 2023-07-24 07:46:08 +00:00
achuprov force-pushed master from 719973b3fa to 53d601603d 2023-07-24 13:54:19 +00:00 Compare
fyrchik approved these changes 2023-07-24 13:54:24 +00:00
dstepanov-yadro merged commit 53d601603d into master 2023-07-24 14:24:31 +00:00
Sign in to join this conversation.
No description provided.