metalinter: address goconst warnings#15566
Conversation
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
karalabe
left a comment
karalabe
left a comment
There was a problem hiding this comment.
LGTM, will merge when green. Thanks!
There was a problem hiding this comment.
IMHO this change is not an improvement.
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
3 participants
added new lines for the config array since it'll keep growing