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

Enable same linters as the Prometheus repo itself #1056

Merged
merged 6 commits into from Jun 17, 2022

Conversation

sazary
Copy link
Contributor

@sazary sazary commented May 21, 2022

This will enable same linters in golangci-lint as the base Prometheus repo, and fixes current issues that linters complain about.
I've copied the config in that repo, specifically this version and removed some parts:

  • I've removed depguard because I don't know what are the policies in this repo about the dependencies, and the requirements of the source repo are not met here
  • In the source config the generated files are all excluded from linting, but we don't have a good naming convention for filenames of generated files here.

I've split the changes applied for each linter into separate commits so that it's easier for review. I think the most critical changes that need review are in the 3dfd8d0 where I've fixed errcheck issues.

Signed-off-by: sazary soroosh@azary.ir

Signed-off-by: sazary <soroosh@azary.ir>
Signed-off-by: sazary <soroosh@azary.ir>
Signed-off-by: sazary <soroosh@azary.ir>
Signed-off-by: sazary <soroosh@azary.ir>
Signed-off-by: sazary <soroosh@azary.ir>
Signed-off-by: sazary <soroosh@azary.ir>
@sazary
Copy link
Contributor Author

sazary commented May 21, 2022

@bwplotka @kakkoyun could you please take the time to review this? thanks

Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

This looks great! I'm happy to merge it. And sorry for the late response :)

I have a couple of questions left. I would be glad if you can clarify those.

max-same-issues: 0
exclude-rules:
- path: _test.go
linters:
Copy link
Member

Choose a reason for hiding this comment

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

These linters don't seem to be enabled, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

golangci-lint has a list of default linters that are enabled by default, unless being disabled explicitly or implicitly. in the previous config they were disabled implicitly by using disable-all: true in this line

the default linters list is:

  • deadcode
  • errcheck
  • gosimple
  • govet
  • ineffassign
  • staticcheck
  • structcheck
  • typecheck
  • unused
  • varcheck

since in the main prometheus repo they left them enabled implicitly i did the same too but i understand that it could reduce readability.

do you want me to enable them in the config explicitly or leave them as they are?

Copy link
Member

Choose a reason for hiding this comment

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

Having them explicit would be amazing. Also, this would prevent any unwanted issues, if we upgrade golangci-lint and decide to change their default preset.

@@ -72,7 +72,8 @@ const (
//
// The current default is GoRuntimeMemStatsCollection, so the compatibility mode with
// client_golang pre v1.12 (move to runtime/metrics).
func WithGoCollections(flags GoCollectionOption) goOption {
//nolint:staticcheck // Ignore SA1019 until v2.
Copy link
Member

Choose a reason for hiding this comment

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

Is staticcheck enabled? From the config above, it doesn't seem like it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as #1056 (comment)

@sazary sazary requested a review from kakkoyun June 2, 2022 17:20
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

LGTM.

Let's merge this and see how it goes!

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.

None yet

2 participants