Skip to content

gocritic: update default checks list && fix new added gocritic linting errs #342

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

Merged
merged 3 commits into from
Jan 8, 2019
Merged

gocritic: update default checks list && fix new added gocritic linting errs #342

merged 3 commits into from
Jan 8, 2019

Conversation

lopezator
Copy link
Contributor

@lopezator lopezator commented Dec 28, 2018

  • Update gocritic default checks list, add the current gocritic stable checks list.
  • Fix gocritic new added linters detected problems.

Closes #341

Update gocritic default checks list, add the current gocritic stable checks list
@lopezator
Copy link
Contributor Author

CC/ @jirfag not sure why the the check is failing. It looks that make readme added a version flag that was missing (and I didn't changed). Maybe this could be the cause?

@jirfag
Copy link
Contributor

jirfag commented Jan 3, 2019

thank you for the pull request! I guess you've used installed binary of golangci-lint in generation of README.md, therefore --version was added. In CI we check generation by locally installed code without --version flag. I've fixed it.

@lopezator
Copy link
Contributor Author

lopezator commented Jan 3, 2019

Thanks @jirfag! It looks like we've have now "gocritic" checks that are not passing.

If you want to go forward with this I could update this with a new commit including the needed changes.

Fix code to pass newly added gocritic checks, mainly pointer receivers and import shadows
@@ -237,18 +237,18 @@ func (m Manager) GetAllEnabledByDefaultLinters() []linter.Config {
return ret
}

func linterConfigsToMap(lcs []linter.Config) map[string]*linter.Config {
func linterConfigsToMap(lcs []*linter.Config) map[string]*linter.Config {
ret := map[string]*linter.Config{}
for _, lc := range lcs {
lc := lc // local copy
Copy link
Contributor Author

@lopezator lopezator Jan 3, 2019

Choose a reason for hiding this comment

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

@jirfag Is this local copy necessary now if we use pointers?

Copy link
Contributor

@jirfag jirfag Jan 8, 2019

Choose a reason for hiding this comment

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

it's for scopelint I guess - it finds a lot of false-positives

@lopezator
Copy link
Contributor Author

PTAL @jirfag

@lopezator lopezator changed the title gocritic: update default checks list gocritic: update default checks list && fix new added gocritic linting errs Jan 3, 2019
@jirfag jirfag merged commit d9a1bdb into golangci:master Jan 8, 2019
@jirfag
Copy link
Contributor

jirfag commented Jan 8, 2019

I was looking too long because I had doubts do we really need to enable performance checks in go-critic by default. Some developers can say that fixing them is a premature optimization. And a lot of users just use --enable-all and go get and will get these performance warnings after merging this PR. But I decided it's more useful than not.
Thank you very much, great work, this pointers refactoring was long needed

@lopezator
Copy link
Contributor Author

lopezator commented Jan 8, 2019

@jirfag I understand, but IMHO the only way to stay safe is to disable all linters by default. Then define all your linters + checks manually.

I agree that this would change "enable-all" users current behaviour, but also it will the inclusion of a new default enabled check (gocritic was enabled) and even more drastically.

Maybe this information should be added somewhere in the docs to warn new users.

@lopezator
Copy link
Contributor Author

Btw I have some ideas to face versioning and breaking changes. I could give more details or propose something when I have more time :)

@jirfag
Copy link
Contributor

jirfag commented Jan 8, 2019

I've already warned in README about it, it doesn't help :)
During an update of gocritic version I've found that it's behavior doesn't match our behavior: gocritic doesn't enable performance checkers by default. I decided that we should be in sync to not confuse users. Also, I've removed copy-pasting a list of checks. PTAL this #353 pull request

@ldez ldez added this to the v1.13 milestone Mar 6, 2024
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.

gocritic enabled by default check list
3 participants