Skip to content

chore: enable and fixes rules from gocritic - #246

Open
mmorel-35 wants to merge 5 commits into
moby:mainfrom
mmorel-35:gocritic
Open

chore: enable and fixes rules from gocritic#246
mmorel-35 wants to merge 5 commits into
moby:mainfrom
mmorel-35:gocritic

Conversation

@mmorel-35

Copy link
Copy Markdown

Enables all gocritic rules by default, then excludes the one non compliant then fix one rule per commit
Covers #245 gocritic part only

Comment thread .golangci.yml Outdated
Comment thread .golangci.yml Outdated
@kolyshkin

Copy link
Copy Markdown
Collaborator

Overall LGTM, left some nits.

Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>
Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>
Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>
Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>
Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>
Comment thread .golangci.yml
Comment on lines +16 to +26
disabled-checks:
- appendAssign
- builtinShadow
- deferInLoop
- hugeParam
- importShadow
- paramTypeCombine
- rangeValCopy
- sloppyReassign
- unnamedResult
- whyNoLint

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the intent to address all of these (in follow-ups), or are there that cannot (yet) be addressed (e.g. due to go versions).

It's good to leave a comment for each to outline if it's either a permanent (won't fix because) or temporary (TODO).

At least some of them stood out as "should likely be fixed" (whyNoLint, rangeValCopy, importShadow), and I know the builtinShadow is a bit too opinionated (it's "OK", but sometimes not worth the code-churn.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you already know if you don't want some of them ? Otherwise the intention is indeed to try to comply with those that are the moby group and the sys project particularly usual practices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants