Skip to content

Add nolintlint linter as internal linter #837

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 2 commits into from
Apr 30, 2020

Conversation

ashanbrown
Copy link
Contributor

This is a rework of #740 with the nolintlint package moved into golangci-lint. This also tries to address #832. It works by creating possible unused issues in the nolintlint linter and then filters them out in the nolint processor. I realize this is a debatable approach to the problem but it does allow the decoupling of analysis and the nolint processor (and inadvertently allows caching of the nolintlint analysis).

The alternate approach to this is to move all the logic into the nolint processor. I'd be happy to do that if that is the preferred approach. Please let me know your thoughts.

Finally, this PR removes all the unused nolint statements in golangci-lint. That could be done in another PR but it is interesting to see what the effect of this is.

@ashanbrown ashanbrown marked this pull request as ready for review November 6, 2019 14:54
@tpounds tpounds added the linter: new Support new linter label Dec 29, 2019
@ashanbrown
Copy link
Contributor Author

Greetings, was wondering if this is still something you would consider? I’d be happy to update if you are. Please let me know. Thank you.

@ernado
Copy link
Member

ernado commented Feb 17, 2020

Hi, sorry for lack of feedback, seems like no maintainer is currently free to review new features :(
I hope that it will change soon, but currently we are (or at least myself) trying to focus on bugfixes.

Thank you for your PR and sorry again.

Copy link
Member

@ernado ernado left a comment

Choose a reason for hiding this comment

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

Hi! Thank you again.
Can you please rebase so I can review it and merge?
It looks LGTM, the only con I see that it partially duplicate gocritic check for nolint without comments.

@ashanbrown ashanbrown force-pushed the asb/nolintlint-internal branch 3 times, most recently from ecfeabb to 60092d9 Compare April 27, 2020 03:11
Linter can check that nolint statements are properly formatted and also that all
nolint statements are used.
@ashanbrown ashanbrown force-pushed the asb/nolintlint-internal branch from 60092d9 to 909f628 Compare April 27, 2020 03:20
@ashanbrown ashanbrown requested a review from ernado April 27, 2020 03:27
@ashanbrown ashanbrown changed the title Add nolintlint lintern as internal linter Add nolintlint linter as internal linter Apr 27, 2020
@ernado ernado merged commit eeff390 into golangci:master Apr 30, 2020
@ldez ldez added this to the v1.26 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linter: new Support new linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants