const-check #4

Merged
dstepanov-yadro merged 3 commits from achuprov/linters:const-check into master 2024-09-04 19:51:22 +00:00
Member

Add check for source of constants

Add check for source of constants
achuprov added 1 commit 2023-07-27 13:21:38 +00:00
linters: refactoring
All checks were successful
Tests and linters / Build lib (pull_request) Successful in 53s
Tests and linters / Staticcheck (pull_request) Successful in 1m2s
Tests and linters / Tests (pull_request) Successful in 2m11s
Tests and linters / Lint (pull_request) Successful in 6m51s
53d601603d
achuprov changed title from const-check to WIP: const-check 2023-07-27 13:24:49 +00:00
achuprov requested review from dstepanov-yadro 2023-07-27 13:25:14 +00:00
achuprov force-pushed const-check from 2be7deb992 to f44444299c 2023-08-03 10:39:48 +00:00 Compare
achuprov force-pushed const-check from f44444299c to b51088a244 2023-08-03 11:08:16 +00:00 Compare
achuprov force-pushed const-check from b51088a244 to de6df5678a 2023-08-03 11:12:55 +00:00 Compare
achuprov changed title from WIP: const-check to const-check 2023-08-03 11:16:44 +00:00
fyrchik reviewed 2023-08-03 11:52:14 +00:00
README.md Outdated
@ -25,0 +23,4 @@
original-url: git.frostfs.info/TrueCloudLab/linters.git
settings:
target-methods : ["reportFlushError", "reportError"] #optional. Enabled by default "Debug", "Info", "Warn", "Error"
constants-repository: "git.frostfs.info/rep/logs" #if not set, then the check is disabled
Owner

Isn't it a package, not repository?

Isn't it a `package`, not `repository`?
fyrchik marked this conversation as resolved
README.md Outdated
@ -44,6 +47,7 @@ Add to .golangci.yml
path: <Path to the directory with libraries>
original-url: git.frostfs.info/TrueCloudLab/linters
Owner

?

?
fyrchik marked this conversation as resolved
@ -39,1 +46,3 @@
if len(expr.Args) == 0 || !isStringValue(expr.Args[0]) {
if !isStringValue(expr.Args[0]) {
alias, _ := getAliasByRepName(file, Config.ConstantsRepository)
if Config.ConstantsRepository == "" || isConstLogsField(expr.Args[0], alias) || getPackageName(expr.Args[0]) == "" {
Owner

It seems we use isConstLogsFIeld only here, and we still do getPackageName inside and on this line. What about removing isConstLogsField and using real == alias || real == ""?

It seems we use `isConstLogsFIeld` only here, and we still do `getPackageName` inside and on this line. What about removing `isConstLogsField` and using `real == alias || real == ""`?
fyrchik marked this conversation as resolved
@ -40,0 +53,4 @@
Pos: expr.Pos(),
End: expr.End(),
Category: LinterName,
Message: "Source of constants cannot be resolved",
Owner

What kind of resolving problems can we have here? Isn't it just wrong package for constants?

What kind of resolving problems can we have here? Isn't it just `wrong package for constants`?
fyrchik marked this conversation as resolved
@ -85,0 +121,4 @@
return alias, nil
}
func getImportSpecs(file *ast.File) []*ast.ImportSpec {
Owner

What about file.Imports?

What about `file.Imports`?
Author
Member

*ast.File.Imports is a slice, it can't be used as a key. Thus, we would have to add a second argument solely to use it as a key. Should I do it this way?

*ast.File.Imports is a slice, it can't be used as a key. Thus, we would have to add a second argument solely to use it as a key. Should I do it this way?
Owner

No, I mean isn't is the case that file.Imports == getImportSpecs(file)?

No, I mean isn't is the case that `file.Imports == getImportSpecs(file)`?
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
@ -85,0 +151,4 @@
return ""
}
split := strings.Split(importName, "/")
Owner

Have you checked what happens when the directory is pkg/xxx and there is package yyy file inside?

Have you checked what happens when the directory is `pkg/xxx` and there is `package yyy` file inside?
Author
Member

I tested it in the case with aliases. If I understood correctly, this is the same situation

I tested it in the case with aliases. If I understood correctly, this is the same situation
Owner

I mean https://go.dev/ref/spec#Import_declarations

If the PackageName is omitted, it defaults to the identifier specified in the package clause of the imported package.

It seem you use directory name instead here.

I mean https://go.dev/ref/spec#Import_declarations >If the PackageName is omitted, it defaults to the identifier specified in the package clause of the imported package. It seem you use _directory_ name instead here.
achuprov force-pushed const-check from de6df5678a to 6cff064b02 2023-08-03 15:17:07 +00:00 Compare
dstepanov-yadro requested changes 2023-08-04 07:42:09 +00:00
@ -85,0 +154,4 @@
split := strings.Split(importName, "/")
alias := split[len(split)-1]
if spec.Name != nil {

Looks like this condition can be checked before replace and split.

Looks like this condition can be checked before replace and split.
Author
Member

fixed

fixed
dstepanov-yadro marked this conversation as resolved
achuprov force-pushed const-check from 6cff064b02 to 6e43f24a57 2023-08-04 08:40:23 +00:00 Compare
achuprov requested review from fyrchik 2023-08-04 08:43:38 +00:00
achuprov force-pushed const-check from 6e43f24a57 to fddd52a332 2023-08-04 10:42:00 +00:00 Compare
achuprov force-pushed const-check from fddd52a332 to 0c3e5e8d68 2023-08-04 11:23:08 +00:00 Compare
achuprov force-pushed const-check from 0c3e5e8d68 to 420dd98c24 2023-08-04 11:26:46 +00:00 Compare
Author
Member
https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/526#issuecomment-17924
dstepanov-yadro approved these changes 2023-08-07 06:18:37 +00:00
achuprov added 1 commit 2023-08-07 14:31:57 +00:00
Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
achuprov force-pushed const-check from 6a00a875e4 to 1c1b1596a8 2023-08-07 14:36:52 +00:00 Compare
fyrchik approved these changes 2023-08-07 15:02:09 +00:00
.golangci.yml Outdated
@ -62,3 +62,3 @@
- contextcheck
disable-all: true
fast: false
fast: false
Owner

What has changed here?

What has changed here?
@ -39,1 +47,3 @@
if len(expr.Args) == 0 || !isStringValue(expr.Args[0]) {
if !isStringValue(expr.Args[0]) {
alias, _ := getAliasByPkgName(file, Config.ConstantsPackage)
if Config.ConstantsPackage == "" || getPackageName(expr.Args[0]) == alias || getPackageName(expr.Args[0]) == "" {
Owner

It could be better to cache it, but ok.

It could be better to cache it, but ok.
fyrchik marked this conversation as resolved
achuprov force-pushed const-check from 1c1b1596a8 to bf126a4841 2023-08-07 16:05:19 +00:00 Compare
achuprov force-pushed const-check from bf126a4841 to 75a7ce14d6 2023-08-07 16:07:30 +00:00 Compare
achuprov force-pushed const-check from 75a7ce14d6 to 7ceda9eddf 2023-08-07 16:12:28 +00:00 Compare
dstepanov-yadro approved these changes 2023-08-07 16:15:22 +00:00
dstepanov-yadro merged commit 7ceda9eddf into master 2023-08-07 16:15:30 +00:00
Sign in to join this conversation.
No description provided.