metalinter: address goconst warnings #15566
Conversation
build/ci.go
Outdated
| configs := []string{"--vendor", "--disable-all", "--enable=vet", "--enable=gofmt", "--enable=misspell"} | ||
| configs := []string{ | ||
| "--vendor", | ||
| "--deadline=10m", |
There was a problem hiding this comment.
Please don't expand the fast linters' runtime. Anything that takes a lot of time to run should be placed below in the slow linter section beside unconvert.
There was a problem hiding this comment.
oops, that was a vestige of running it in a low memory environment. fixed
zramsay
force-pushed
the
linter-goconst
branch
from
57e2744 to
2424028
Compare
internal/build/env.go
Outdated
| const ( | ||
| travisTrue = "true" | ||
| appveyorTrue = "True" | ||
| ) |
There was a problem hiding this comment.
IMHO this change is not an improvement.
node/config.go
Outdated
| @@ -41,6 +41,9 @@ const ( | |||
| datadirStaticNodes = "static-nodes.json" // Path within the datadir to the static node list | |||
| datadirTrustedNodes = "trusted-nodes.json" // Path within the datadir to the trusted node list | |||
| datadirNodeDatabase = "nodes" // Path within the datadir to store the node infos | |||
|
|
|||
| lowerCaseGeth = "geth" | |||
| upperCaseGeth = "Geth" | |||
There was a problem hiding this comment.
is there a better way of addressing:
internal/build/env.go:66:26:warning: 2 other occurrence(s) of "True" found in: internal/build/env.go:66:61 internal/build/env.go:75:60 (goconst)
internal/build/env.go:66:61:warning: 2 other occurrence(s) of "True" found in: internal/build/env.go:66:26 internal/build/env.go:75:60 (goconst)
internal/build/env.go:75:60:warning: 2 other occurrence(s) of "True" found in: internal/build/env.go:66:26 internal/build/env.go:66:61 (goconst)
node/config.go:219:13:warning: 2 other occurrence(s) of "geth" found in: node/config.go:263:17 node/config.go:265:16 (goconst)
node/config.go:263:17:warning: 2 other occurrence(s) of "geth" found in: node/config.go:219:13 node/config.go:265:16 (goconst)
node/config.go:265:16:warning: 2 other occurrence(s) of "geth" found in: node/config.go:219:13 node/config.go:263:17 (goconst)
?
There was a problem hiding this comment.
could use // nolint: goconst but that sort of defeats the purpose IMO
There was a problem hiding this comment.
@karalabe may differ, but I feel goconst should not be enabled on CI because it reports things like this. We can enable ineffassign, nakedret, etc. because they're genuinely useful.
There was a problem hiding this comment.
Given that our code is generally clean, maybe I can agree with @fjl to keep this off. Perhaps reduce this PR to fixing the swarm repetition and it will be fine like that.
There was a problem hiding this comment.
I'm OK with adding goconst if we raise --min-occurrences to 6 or 7. There is value in using constants for repeated strings, but IMHO it's something that jumps in your face anyway while reviewing the code.
There was a problem hiding this comment.
good points; added the flag (which is suppoorted by metalinter itself) and reverted the code
zramsay
force-pushed
the
linter-goconst
branch
3 times, most recently
from
9ce308e to
58be787
Compare
zramsay
force-pushed
the
linter-goconst
branch
from
58be787 to
c55869c
Compare
added new lines for the config array since it'll keep growing