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

Disable all first then enable needed linters #439

Merged
merged 1 commit into from
Dec 30, 2019

Conversation

ferhatelmas
Copy link
Contributor

@ferhatelmas ferhatelmas commented Dec 18, 2019

First disable all linters then enable what we need. Otherwise, all linters are running then results are filtered. See with -v flag.

Also, sort enabled linters in config.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 18, 2019
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 18, 2019
@ferhatelmas ferhatelmas changed the title Disable first then enable linters Disable all first then enable needed linters Dec 18, 2019
@codecov-io
Copy link

codecov-io commented Dec 18, 2019

Codecov Report

Merging #439 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #439   +/-   ##
=======================================
  Coverage   56.25%   56.25%           
=======================================
  Files          19       19           
  Lines         928      928           
=======================================
  Hits          522      522           
  Misses        351      351           
  Partials       55       55

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7bc0ca...17c1015. Read the comment docs.

@corneliusweig
Copy link
Contributor

I'm not sure I understand what is the problem here. I cannot find any filtering of linter results going on when running with -v.

Besides, I think the enabled linters are correct and some are active by default whereas others are enabled explicitly. Iow, the list of enabled linters in our .golangci.yaml is not exhaustive and does not list what is enabled by default anyways. Does that make sense?

@ferhatelmas
Copy link
Contributor Author

some are active by default

This is the key part. If we don't disable all first, we might be adding more into already enabled ones or our config is noop. However, if we disable first, then the enable list explicitly specifies what is active.

It's somehow related to silence some linters which are useful in the context of high performant server application, but not in a cli such as predeclared.

@corneliusweig
Copy link
Contributor

Alright. Being explicit about the active linters would be nice.

There is some difference between the 17 active linters in this PR (deadcode errcheck gocritic gofmt goimports golint gosimple interfacer maligned misspell prealloc staticcheck structcheck stylecheck unconvert unparam varcheck) vs. the 21 currently active on master (deadcode errcheck gocritic gofmt goimports golint gosimple govet ineffassign interfacer maligned misspell prealloc staticcheck structcheck stylecheck typecheck unconvert unparam unused varcheck). Would you care to clarify why you want to disable those four linters?

@ferhatelmas
Copy link
Contributor Author

It's somehow related to silence some linters which are useful in the context of high performant server application, but not in a cli such as predeclared.

Sorry for the typo, I had meant prealloc, instead of predeclared.

@ferhatelmas
Copy link
Contributor Author

Would you care to clarify why you want to disable those four linters?

Sure, actually my intention was to be explicit and then disable one by one with a discussion.

For this goal to happen, I need to ensure that active linter list isn't modified in this PR. Let me update. However, I won't enable typecheck because it's already covered by tests.

@ahmetb
Copy link
Member

ahmetb commented Dec 30, 2019

/lgtm
/approve
Let's give it a try.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 30, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, ferhatelmas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 30, 2019
@k8s-ci-robot k8s-ci-robot merged commit 885f5ad into kubernetes-sigs:master Dec 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants