-
Notifications
You must be signed in to change notification settings - Fork 589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make the linter config attractive #3486
Conversation
osmomath/sigfig_round.go
Outdated
@@ -15,7 +15,7 @@ func SigFigRound(d sdk.Dec, tenToSigFig sdk.Int) sdk.Dec { | |||
// take note of floor div, vs normal div | |||
k := uint64(0) | |||
dTimesK := d | |||
for ; dTimesK.LT(pointOne); k += 1 { | |||
for ; dTimesK.LT(pointOne); k++ { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which linter said this, can we turn it off
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally, it's revive's increment-decrement, and can get rid of it :)
.golangci.yml
Outdated
# - goerr113 <- disabled due to lack of comprehension | ||
- gofmt | ||
# - gofumpt <- disabled https://github.com/osmosis-labs/osmosis/issues/1309 | ||
- gocritic # <- re enable later to catch capitalized local variables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should delete this, I dont think these are helpful lints.
.golangci.yml
Outdated
- gofmt | ||
# - gofumpt <- disabled https://github.com/osmosis-labs/osmosis/issues/1309 | ||
- gocritic # <- re enable later to catch capitalized local variables | ||
- gofumpt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets leave this deleted, it doesn't have good IDE support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with gofumpt, what I have been doing, is using it to replace both goimports and gofmt
if your ide supports golangci-lint, it will now gofumpt automagically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the lints from gofumpt, I really don't think this is a win to have as overhead on every PR/commit
.golangci.yml
Outdated
- gomodguard | ||
- goprintffuncname | ||
# - gosec <- triggers too much for this round and should be re-enabled later | ||
- gosec # <- triggers too much for this round and should be re-enabled later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be changed in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes.
.golangci.yml
Outdated
# - nlreturn <- disabled as it doesn't auto-fix something simple and doesn't add anything useful | ||
# - noctx <- needs go 1.18 support | ||
# - nolintlint <- disabled because we use nolint in some places | ||
- nolintlint # <- disabled because we use nolint in some places |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets delete this?
Nolints are perfectly fine to have in the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nolintlint just forces us to
//nolint:whatever per-line, I'd prefer to keep it ?
Basically it is an enforcer of //nolint:whatever hygene.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh gothca
Yes to all, except nolintlint? Also, for gofumpt... curious what you mean about ide support? Basically it is now wired into golangci-lint this reduces the whole affair to -- for example -- golangci-lint run ./... --fix in vscode, I set it to do that to my workspace on changes. |
After talking to Adam on slack, I think that this pr should be kept super minimal, and expanded after the concentrated liquidity branch is merged. |
8e44f03
to
2731452
Compare
summary:
|
2dfc733
to
2731452
Compare
We have to fix the lint failures on this PR (or comment out those linters and make a new GH issue for them) |
totally! sorry I missed that :P |
…-labs/osmosis into faddat/fix-ugly-golangci
Closes: #XXX
What is the purpose of the change
Long ago, I used .golangci.yml in Osmosis to learn how to fully configure golangci-lint.
THis PR aims to fix that, while masking changes that Dev mentioned were unnecessary.
Brief Changelog
Testing and Verifying
This change is a trivial rework / code cleanup without any test coverage.
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? (yes / no) nox/<module>/spec/
) / Osmosis docs repo / not documented) not applicable