-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
adhere to the widespread comment directive format #1658
Comments
Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors. |
For the sake of documenting this better upstream, I've raised golang/go#43776. |
Moving to the approach used by staticcheck would also have the advantage that file-wide directives will work properly; currently the way to do a file-wide directed is to attach the comment to the package decl, which in turn results in a line in the godoc. See https://staticcheck.io/docs/#file-based-linter-directives for how staticcheck does it. |
Also, @ldez given that this is a deviation from the Go standard, I think this is a bug and not an enhancement. |
Today,
In the code base of golangci-lint, it's used as a simple comment syntax. So from my point of view, there is no deviation. But my main concern, it's the resulting breaking change of the syntax change.
currently the |
To be honest, I don't think this is a good solution. All golangci-lint directives should clearly look like directives, not plain English comments.
I don't follow - the docs clearly point towards
The fix could be rolled out over time, or come with an automated way to fix up older comments like https://github.com/ipld/specs/pull/353/files#diff-ed097ee1bca871446cf885a8fabf9722c54921cf7bae59f780cd14a6f305c3d5. |
I would propose for golangci-lint to complain about all forms with spaces: |
@AlekSi you can already do that: linters-settings:
nolintlint:
# Disable to ensure that nolint directives don't have a leading space. Default is true.
allow-leading-space: false |
And I think the default should be changed to Edit: also, the current code doesn't handle |
It would be very nice if golangci-lint respected the linter directives of supported linters.
|
A bit tangential here, but I opened #2671 about that. For some linters such as revive and gosec it already works, maybe staticcheck needs special handling or changes. Note that the example here is a bit broken, it has a |
See the upstream bug report above; users are starting to run into this problem now that gofmt reformats godoc comments. I would say that fixing this bug before 1.19 is released in three weeks is very important - otherwise the number of incoming bug reports will multiply. |
I will create a PR to drop It will not be the real fix for this issue but a fix for go1.19 gofmt. In parallel, I started to work on a real fix for this issue with a new syntax. |
I explicitly designed the semantics of that directive to be usable by more tools than just Staticcheck (which is why the prefix is |
I'm talking about the "option" part of the directive: As we check the linter name inside golangci-lint and not the rules inside a linter, we cannot use |
I don't understand what the problem is? If you saw |
I think that adding code related to staticcheck inside the core system of golangci-lint is not a good idea: every linter can use this syntax, if I start with excluding staticcheck directives, I will have to ignore those from other linters. |
I think Dominik means that golangci-lint should ignore directives that it doesn't understand, like |
My point wasn't to ignore Staticcheck's checks specifically, but to simply skip directives concerning checks you do not know. That is, only process those |
Currently, we report The difference between the 2 following lines is only on the content (check name vs linter name): //lint:ignore SA4000
//lint:ignore staticcheck Also, I think that users will not understand the point and they will mix both (check name and linter name) in the same line. |
For now, no strong decision, I'm just thinking about it. I have to see what really needs to be kept from the past, and how the behavior can be extended to manage the specific constraints of golangci-lint. The current implementation of So I will not take a decision quickly, and currently, it's not my top priority. |
My linters are colliding already (see: mvdan/gofumpt#241) and I do not believe that waiting for a v2 can actually be the go-to option here, except the v2 release is right around the corner. Why not use the auto-fix option that was proposed previously in this thread? The fact that golangci-lint reports non-existing linters is actually a good thing, because you'll get feedback about the tool setup, otherwise you end up having nolint directives which will only perform half of what they are configured to be doing. Anyway, golangci-lint is not the same use case as staticcheck here, because staticcheck maintains a catalogue of checks, which is less volatile than the linters being added and deprecated within this project. So it actually golangci-lint deserves this sanitization feature and therefore its configuration should live under its own directive domain. PS: A workaround to not break CI pipelines is to enable the fix flag. |
With recent changes `gofmt` [1] started reformatting godoc comments. This causes a problem wherein it reformats `//nolint: staticcheck` to `// nolint: staticcheck`. But it does ignore directives [2]. So let's change all our nolint to directive format. This avoids the conflict with `gofmt`. This fix was done by running: `grep -r --include="*.go" -E "//nolint: .*" -l | xargs sed -i 's/nolint: stylecheck/nolint:stylecheck/g'` as such, we can skip it from review. [1]: golangci/golangci-lint#1658 (comment) [2]: golangci/golangci-lint#3098 (comment)
As can been seen in my comment in issue #2671, which has been closed as its a duplicate of this issue which I hadn't seen, this is still very much a live issue. From reading the comments above it looks there is no solution at present. Is this still the case? Owen |
Using a golangci-lint directive of the form: //nolint:staticcheck // reason why staticcheck has been disabled We can disable the use of the staticcheck linter completely at the blocks or lines where is is currently reporting the use of the deprecated ast.Package stuct. This approach is documented here: https://golangci-lint.run/usage/false-positives/#nolint-directive At present golangci-lint will ignore staticheck //lint:ignore style directives. This issues has been raised with the golangci-lint team. See: golangci/golangci-lint#2671 (comment) and here: golangci/golangci-lint#1658 (comment)
In Go,
//go:*
is reserved for the Go toolchain. See https://golang.org/cmd/compile/.It is encouraged that other Go tools use a similar "namespace" prefix for their comment directives. For example,
//gccgo:*
,//lint:*
for staticcheck,//go-sumtype:*
, and so on. See https://groups.google.com/g/golang-dev/c/r4rdPdsH1Fg/m/tNZazPenX5cJ.golangci-lint kind of does this, but not really. https://golangci-lint.run/usage/false-positives/#nolint gives this example:
That's no good, because it clearly does not adhere to the widespread format. It is not formally defined, but in practice it follows something like
^[a-z]+:[a-z]+
.A possible fix would be
//nolint:all
, which is also clearer. Another option would be to follow staticcheck's https://staticcheck.io/docs/configuration/#line-based-linter-directives, which already follows the format with//lint:ignore ...
.The text was updated successfully, but these errors were encountered: