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.

Sign up for GitHub

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

Jump to bottom

dev: discourage bad practices #803

Merged
merged 1 commit into from Oct 12, 2019
Merged

dev: discourage bad practices #803

merged 1 commit into from Oct 12, 2019

Conversation

jirfag
Copy link
Member

@jirfag jirfag commented Oct 10, 2019

No description provided.

jirfag
jirfag commented Oct 10, 2019
View reviewed changes
go.mod
@@ -40,7 +40,7 @@ require (
github.com/ultraware/whitespace v0.0.4
github.com/uudashr/gocognit v0.0.0-20190926065955-1655d0de0517
github.com/valyala/quicktemplate v1.2.0
golang.org/x/tools v0.0.0-20190930201159-7c411dea38b0
golang.org/x/tools v0.0.0-20191010075000-0337d82405ff
Copy link
Member Author

Choose a reason for hiding this comment

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

it includes my fix golang/tools#162 for those who uses go get: no more ssa panics

lopezator
lopezator approved these changes Oct 10, 2019
View reviewed changes
Copy link
Member

@lopezator lopezator left a comment

Choose a reason for hiding this comment

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

These are a couple of things I am using lately but that took a time to figure them out.

We were using tools.go installation mode until 1.13.

Awesome work @jirfag this adds a lot of value.

.golangci.yml
@@ -48,6 +48,7 @@ linters-settings:
statements: 50

linters:
# please, do not use `enable-all`: it's deprecated and will be removed soon.
Copy link
Member

Choose a reason for hiding this comment

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

I've been using disable-all from the start. This means that from now it's implicit?

Even if we have versioned config it's great in order to avoid unexpected things.

👏

Copy link
Member Author

Choose a reason for hiding this comment

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

I did no backward-incompatible changes: by default some linters are enabled

@jimen0
Copy link
Contributor

jimen0 commented Oct 10, 2019

Hi, where was the idea of deprecating --enable-all discussed?

@jirfag
Copy link
Member Author

jirfag commented Oct 10, 2019

@jimen0 no single place. You can write your thoughts here.

@jimen0
Copy link
Contributor

jimen0 commented Oct 10, 2019

I'd like to keep it there as I enable all linters from command line and then I exclude specific checks from YAML config file. One rule I usually disable is, for example, stylecheck ST1000.

golangci-lint run \
    --verbose \
    --issues-exit-code=1 \
    --exclude-use-default=false \
    --enable-all \
    ./...

@jirfag
Copy link
Member Author

jirfag commented Oct 10, 2019 •
edited

enable-all has the following advantage:

  1. convenient for users, I understand it.
  2. all newly added linters are automatically used by a lot of users.

But it has the following downside: when golangci-lint is updated users typically get some new linters enabled: it can be annoying, it can fail CI builds. Users should disable linters one by one in such cases.

I think long-term updates cost is more important than a one-time configuration cost.

Let's discuss it!

@lopezator
Copy link
Member

IMHO an update shouldn't break any build when you don't expect breaking changes (major version change).

@jimen0
Copy link
Contributor

jimen0 commented Oct 11, 2019

I agree about the cost of disabling new linters each release, @jirfag. In fact I suffer it (locally) each release.
On the other hand, people should not be blindly running --enable-all in CI.

PS: by "suffer" I still mean I love this project

@jirfag jirfag merged commit ca6effb into master Oct 12, 2019
@delete-merged-branch delete-merged-branch bot deleted the feature/discourage-bad-practices branch October 12, 2019 09:36
theckman pushed a commit to theckman/golangci-lint that referenced this pull request May 3, 2020
@jirfag
update go-critic
b31cfd6 
$ git cherry --abbrev -v 0af0999fabfb ee9bf5809ead
+ abd8436 all: enable Go modules on CI (golangci#753)
+ 3c9d0fb checkers: recognize //line and //nolint in commentFormatting (golangci#756)
+ 0b517d7 checkers: extend deprecatedComment patterns (golangci#757)
+ 09100f6 checkers: use astcast package instead of coerce.go (golangci#758)
+ 2e9e97f checker: simplify boolExprSimplify (golangci#759)
+ 575701e make: add go-consistent to CI checks list (golangci#761)
+ b55f431 checkers: fix unlambda handling of builtins (golangci#763)
+ 5a7dee3 checker: handle lambdas properly in boolExprSimplify (golangci#765)
+ 5ce3939 checkers: teach boolExprSimplify a few new tricks (golangci#766)
+ 04d160f checkers: add new patterns to boolExprSimplify (golangci#768)
+ 09582e2 make: collect coverprofile separately from goveralls (golangci#769)
+ d8d0ee4 checkers: recognize NOTE pattern in deprecatedComment (golangci#770)
+ 12f0f85 Update copyright notice to 2019 (golangci#771)
+ f54bdb6 checkers: add stringXbytes checker
+ 170d65c checkers: followup for golangci#773 (golangci#774)
+ 84e9e83 checkers: make stringXbytes more linear (golangci#775)
+ a800815 checkers: add Depreacted typo pattern (golangci#776)
+ 6751be9 checkers: add hexLiterals (golangci#772)
+ ac61906 checkers: add typeAssertChain checker (golangci#782)
+ d19dbf1 checkers: add codegenComment checker (golangci#783)
+ d82b576 checkers: proper pkg/obj check for flagName (golangci#786)
+ dfcf754 ci: enable integration tests (golangci#787)
+ 5dafc45 checkers: fix equalFold false positive (golangci#788)
+ ed5e8e7 checkers: refactor and fix hexLiteral checker (golangci#789)
+ e704e07 checkers: add argOrder checker (golangci#790)
+ 34c1dc8 checkers: add Split handling to argOrder checker (golangci#791)
+ cbe095d checkers: add math.Max and math.Min to dupArg (golangci#792)
+ c986ee5 checkers: add checkers info fields test (golangci#794)
+ 66e5832 cmd/makedocs: use lintpack, fix build (golangci#793)
+ 6bce9d0 cmd/makedocs: add enabled/disabled by default info (golangci#795)
+ 4adbf9a checkers: simplify flagName (golangci#799)
+ 07de34a checkers: add octalLiteral checker (golangci#798)
+ 765907a cmd/makedocs: add checker param docs (golangci#796)
+ ee9bf58 cmd/makedocs: fix headers formatting (golangci#803)
@vearutop
Copy link
Contributor

Please do not deprecate --enable-all, although it has downsides in general usage it is valuable in some cases.

In my projects I usually pinpoint version of golangci-lint so new linters don't come as a surprise. When I manually upgrade a version of golangci-lint I prefer to be compliant with new linters (which happens most of the time). If during upgrade I see a new linter that looks unhelpful - I disable it.

Such approach worked well for me so far, if --enable-all is removed I'll have to become more familiar with every new linter at least because I would need to explicitly decide whether I want to enable it.

@jared2501
Copy link

^ Agree with @vearutop - If we're talking best practices, I think pinning to specific versions for development + CI, rather than pulling depending on the latest version is a pretty important best practice. This fixes the issues of new linters causing issues.

@wyardley wyardley mentioned this pull request Feb 4, 2021
Closed
@alecthomas
Copy link
Contributor

alecthomas commented Feb 9, 2021 •
edited

Yeah I don't quite understand what the issue is.

If you want stability, use disable-all + enable.

I do want this functionality, because I periodically manually bump golangci-lint's version, fix all the new lint issues (or disable linters I don't care about).

If enable-all is removed, what will the replacement upgrade process be? How will we know which linters have been added since the previous version we have without crawling through the changelog?

@ldez
Copy link
Member

ldez commented Feb 9, 2021 •
edited

I recommend discussing in this issue #1686 and #1888

@ldez ldez mentioned this pull request Apr 4, 2021
Closed
@ldez ldez added this to the v1.21 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lopezator lopezator lopezator approved these changes

@tpounds tpounds Awaiting requested review from tpounds

@ernado ernado Awaiting requested review from ernado

@matoous matoous Awaiting requested review from matoous

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jirfag @jimen0 @lopezator @vearutop @jared2501 @alecthomas @ldez