Skip to content
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

Update gochecknoglobals #1393

Closed
leighmcculloch opened this issue Sep 24, 2020 · 4 comments · Fixed by #1422
Closed

Update gochecknoglobals #1393

leighmcculloch opened this issue Sep 24, 2020 · 4 comments · Fixed by #1422
Assignees
Labels
enhancement New feature or improvement

Comments

@leighmcculloch
Copy link
Contributor

Is your feature request related to a problem? Please describe.
It looks like in response to #199 gochecknoglobals was ported into golangci-lint, but it's also out-of-date, as the latest gochecknoglobals makes some new exceptions.

Describe the solution you'd like
Would it be possible for golandci-lint to call out to gochecknoglobals to make updating easier here, or is there a pattern I can follow to make it possible to hook into so you can simply import and call?

Alternatively the implementation in golangci-lint needs updating to pickup the new changes to maintain consistency.

@leighmcculloch leighmcculloch added the enhancement New feature or improvement label Sep 24, 2020
@boring-cyborg
Copy link

boring-cyborg bot commented Sep 24, 2020

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

@bombsimon
Copy link
Member

bombsimon commented Sep 24, 2020

golangci-lint always vendors it's dependencies but doing so is still easier than copying the linter like this. However, golangci-lint relies on the analysis.Analyzer so the linter must support that. There are to ways to do that:

  1. Make gochecknoglobals return both the error message and the token.Pos so golangci-lint can use this and add it to an internal analyzer like this or this.
  2. Make gochecknoglobals implement it's own analysis.Analyzer and it could be used as is in golangci-lint like this (see source).

I think 2 would be pretty easy to implement to gochecknoglobals and would make it super easy to maintain in golangci-lint by just bumping the dependency version.

EDIT: golangci-lint does no longer vendor it's dependencies but uses the modules proxy. The effect is the same though, we should use the external analyser.

@leighmcculloch
Copy link
Contributor Author

I agree I think 2 would be the best forward looking approach. Thanks @bombsimon for pointing this out.

@leighmcculloch
Copy link
Contributor Author

Thanks to @bombsimon the gochecknoglobals tool is now built on the Go analysis package. I can make the same changes to the gochecknoinits tool too so it can also be upgraded easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants