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

nollint rule should be written without leading space as //nolint conflicts with gofmt #3109

Closed
4 tasks done
ldemailly opened this issue Aug 18, 2022 · 4 comments
Closed
4 tasks done
Labels
duplicate This issue or pull request already exists

Comments

@ldemailly
Copy link

Welcome

  • Yes, I'm using a binary release within 2 latest major releases. Only such installations are supported.
  • Yes, I've searched similar issues on GitHub and didn't find any.
  • Yes, I've included all information below (version, config, etc).
  • Yes, I've tried with the standalone linter if available. (https://golangci-lint.run/usage/linters/)

Description of the problem

VSCode and the gofmt linters force the comments to be

// nolint:...

(one space) and nolintlint wants no space

gofmt probably should win

so basically the default config in golangci-lint should not be

    allow-leading-space: false

because it conflicts with gofmt (and the same rules)

Version of golangci-lint

$ golangci-lint --version
golangci-lint --version
golangci-lint has version 1.48.0 built from 2d8fea8 on 2022-08-04T09:19:19Z

Configuration file

  nolintlint:
    require-specific: true

Go environment

both go1.18.5 and go.19 on both linux amd 64 and macos arm64

Verbose output of running

$ golangci-lint run -v ./log/logger.go 
INFO [config_reader] Config search paths: [./ /Users/ldemailly/dev/fortio/log /Users/ldemailly/dev/fortio /Users/ldemailly/dev /Users/ldemailly /Users /] 
INFO [config_reader] Used config file .golangci.yml 
INFO [lintersdb] Active 71 linters: [asasalint asciicheck bidichk bodyclose containedctx contextcheck deadcode decorder depguard dogsled dupl durationcheck errcheck errchkjson errname errorlint execinquery exhaustive exportloopref funlen gci gocognit goconst gocritic gocyclo godot godox goerr113 gofmt gofumpt goheader goimports gomoddirectives gomodguard goprintffuncname gosec gosimple govet grouper importas ineffassign lll maintidx makezero misspell nakedret nestif nilerr nilnil noctx nolintlint nosnakecase nosprintfhostport prealloc predeclared promlinter revive rowserrcheck sqlclosecheck staticcheck structcheck stylecheck tenv tparallel typecheck unconvert unparam unused usestdlibvars varcheck whitespace] 
INFO [loader] Go packages loading at mode 575 (files|deps|exports_file|imports|name|types_sizes|compiled_files) took 116.234167ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 506.375µs 
WARN [linters context] contextcheck is disabled because of generics. You can track the evolution of the generics support by following the https://github.com/golangci/golangci-lint/issues/2649. 
INFO [linters context] importas settings found, but no aliases listed. List aliases under alias: key. 
INFO [linters context/goanalysis] analyzers took 1.717282986s with top 10 stages: buildir: 669.016333ms, exhaustive: 155.261957ms, fact_deprecated: 90.792418ms, ctrlflow: 88.694206ms, nilness: 80.372291ms, fact_purity: 78.631995ms, SA5012: 75.354037ms, typedness: 72.952666ms, findcall: 70.979789ms, printf: 65.787421ms 
WARN [linters context] rowserrcheck is disabled because of generics. You can track the evolution of the generics support by following the https://github.com/golangci/golangci-lint/issues/2649. 
WARN [linters context] sqlclosecheck is disabled because of generics. You can track the evolution of the generics support by following the https://github.com/golangci/golangci-lint/issues/2649. 
WARN [linters context] structcheck is disabled because of generics. You can track the evolution of the generics support by following the https://github.com/golangci/golangci-lint/issues/2649. 
INFO [runner] Issues before processing: 10, after processing: 1 
INFO [runner] Processors filtering stat (out/in): skip_files: 10/10, exclude-rules: 8/10, nolint: 1/8, max_same_issues: 1/1, path_shortener: 1/1, path_prettifier: 10/10, exclude: 10/10, max_from_linter: 1/1, source_code: 1/1, severity-rules: 1/1, skip_dirs: 10/10, identifier_marker: 10/10, uniq_by_line: 1/1, sort_results: 1/1, cgo: 10/10, autogenerated_exclude: 10/10, diff: 1/1, max_per_file_from_linter: 1/1, path_prefixer: 1/1, filename_unadjuster: 10/10 
INFO [runner] processing took 1.050709ms with stages: nolint: 371.667µs, exclude-rules: 331.375µs, identifier_marker: 138.167µs, autogenerated_exclude: 123.959µs, path_prettifier: 38.042µs, source_code: 34.417µs, skip_dirs: 5.958µs, uniq_by_line: 2.042µs, cgo: 1.166µs, filename_unadjuster: 833ns, path_shortener: 500ns, max_same_issues: 500ns, severity-rules: 458ns, max_per_file_from_linter: 375ns, sort_results: 291ns, max_from_linter: 250ns, exclude: 209ns, diff: 209ns, skip_files: 208ns, path_prefixer: 83ns 
INFO [runner] linters took 1.313440334s with stages: goanalysis_metalinter: 1.312274042s, contextcheck: 6.291µs, rowserrcheck: 5.583µs, sqlclosecheck: 3.125µs, structcheck: 2.334µs 
log/logger.go:67:1: directive `// nolint: gochecknoinits // needed` should be written without leading space as `//nolint: gochecknoinits // needed` (nolintlint)
// nolint: gochecknoinits // needed
^
INFO File cache stats: 2 entries of total size 15.0KiB 
INFO Memory: 16 samples, avg is 180.0MB, max is 247.3MB 
INFO Execution took 1.438874417s                  

Code example or link to a public repository

https://github.com/fortio/fortio/blob/master/log/logger.go#L67

@ldemailly ldemailly added the bug Something isn't working label Aug 18, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Aug 18, 2022

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

@ldez
Copy link
Member

ldez commented Aug 18, 2022

Hello,

Due to a change of go fmt inside go1.19.
We have to enforce the directive style.

So the option allow-leading-space has been dropped.

The syntax of directives is: [a-zA-Z]+:[a-zA-Z].*.
The spaces around : and after the // must be removed.

// nolint:foo // <-- malformed
//nolint :foo // <-- malformed
//nolint: foo // <-- malformed

//nolint:foo // <-- OK

If you do that your IDE or go fmt will format the directive as a directive and it will not add extra space.

Related to:
#3002
#1658 (comment)

duplicate of #3098, #3063, #3068, #3101.

@ldez ldez closed this as completed Aug 18, 2022
@ldez ldez added duplicate This issue or pull request already exists and removed bug Something isn't working labels Aug 18, 2022
@ldemailly
Copy link
Author

Thanks and sorry for the dup. Just to clarify for others:

Search and replace
// nolint: with //nolint:

The conflict issue I had was keeping the space after nolint: (which isn't super obvious from your reply)

@ldemailly
Copy link
Author

also it's a bit of an ugly rule because

                      if event.Name == u.dirPath || event.Name == path.Join(u.dirPath, k8sDataSymlink) { //nolint: nestif

is fine/accepted why is that space ok

clementnuss added a commit to postfinance/kubelet-csr-approver that referenced this issue Aug 23, 2022
clementnuss added a commit to postfinance/kubelet-csr-approver that referenced this issue Aug 29, 2022
r-vasquez added a commit to r-vasquez/redpanda that referenced this issue Sep 8, 2022
There was a change for nolint directives:
golangci/golangci-lint#3109 (comment)

And we are adding confidence of 0.8 in revive to
avoid false positives.
r-vasquez added a commit to r-vasquez/redpanda that referenced this issue Sep 9, 2022
There was a change for nolint directives:
golangci/golangci-lint#3109 (comment)

And we are adding confidence of 0.8 in revive to
avoid false positives.
r-vasquez added a commit to r-vasquez/redpanda that referenced this issue Sep 13, 2022
There was a change for nolint directives:
golangci/golangci-lint#3109 (comment)

And we are adding confidence of 0.8 in revive to
avoid false positives.
ballard26 pushed a commit to ballard26/redpanda that referenced this issue Sep 27, 2022
There was a change for nolint directives:
golangci/golangci-lint#3109 (comment)

And we are adding confidence of 0.8 in revive to
avoid false positives.
viccuad added a commit to viccuad/kubewarden-controller that referenced this issue Oct 4, 2022
See also:
golangci/golangci-lint#3109

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
viccuad added a commit to viccuad/kubewarden-controller that referenced this issue Oct 11, 2022
See also:
golangci/golangci-lint#3109

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
jvanz pushed a commit to jvanz/kubewarden-controller that referenced this issue Dec 13, 2022
See also:
golangci/golangci-lint#3109

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
joejulian pushed a commit to joejulian/redpanda that referenced this issue Mar 10, 2023
There was a change for nolint directives:
golangci/golangci-lint#3109 (comment)

And we are adding confidence of 0.8 in revive to
avoid false positives.
joejulian pushed a commit to joejulian/redpanda that referenced this issue Mar 24, 2023
There was a change for nolint directives:
golangci/golangci-lint#3109 (comment)

And we are adding confidence of 0.8 in revive to
avoid false positives.
joejulian pushed a commit to joejulian/redpanda that referenced this issue Apr 12, 2023
There was a change for nolint directives:
golangci/golangci-lint#3109 (comment)

And we are adding confidence of 0.8 in revive to
avoid false positives.

(cherry picked from commit bf5401e)
joejulian pushed a commit to joejulian/redpanda that referenced this issue Apr 12, 2023
There was a change for nolint directives:
golangci/golangci-lint#3109 (comment)

And we are adding confidence of 0.8 in revive to
avoid false positives.

(cherry picked from commit bf5401e)
joejulian pushed a commit to joejulian/redpanda that referenced this issue Apr 13, 2023
There was a change for nolint directives:
golangci/golangci-lint#3109 (comment)

And we are adding confidence of 0.8 in revive to
avoid false positives.

(cherry picked from commit bf5401e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

2 participants