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

chore: make nolint in its own line #446

Merged
merged 4 commits into from
Feb 10, 2025
Merged

Conversation

cuixq
Copy link
Collaborator

@cuixq cuixq commented Feb 10, 2025

Tweak the comments a bit to make it pass both the public and internal linters.

@cuixq cuixq changed the title chore: remove nolint chore: disable nolintlint Feb 10, 2025
@cuixq cuixq marked this pull request as ready for review February 10, 2025 00:49
@cuixq cuixq requested a review from another-rex February 10, 2025 00:49
@G-Rath
Copy link
Collaborator

G-Rath commented Feb 10, 2025

nolintlint is a pretty good lint for helping managing our linting so I'm pretty against turning it off - in saying that, are you sure you've not missed something? as we have other //nolint comments in the codebase that have not been flagged as an issue, and my understanding is that it's based on Go comment directives which are required to have no space between the comment slashes and the text, so I don't see how you can require a space in all comments?

This is the only //nolint that has a message, so I'm thinking maybe the internal linter is designed to ignore Go comment directives which it identifies based on a space - so changing the comment in question to have it's explainer on a dedicated line might be the fix

i.e. instead of

//nolint:nilerr // continue walking if there is an error

do this

// continue walking if there is an error
//nolint:nilerr

@cuixq
Copy link
Collaborator Author

cuixq commented Feb 10, 2025

From my experience working on #399, //nolint:nilerr will still break the internal checker so that's why I added a space in this commit.

@G-Rath
Copy link
Collaborator

G-Rath commented Feb 10, 2025

but then why isn't it complaining about the other //nolint comments in the codebase?

osv-scalibr on  main [?] via 🐹 v1.23.4 via  v20.11.0
❯ rg nolint
testing/extracttest/errors.go
25://nolint:errname

binary/cdx/cdx_test.go
31://nolint:gochecknoinits

testing/fakeextractor/fake_extractor_test.go
145:            //nolint:containedctx

internal/datasource/http_auth.go
210:            ha1 := md5.Sum([]byte(auth.Username + ":" + realm + ":" + auth.Password)) //nolint:gosec
219:                    ha1 = md5.Sum(b.Bytes()) //nolint:gosec
233:            ha2 := md5.Sum([]byte(req.Method + ":" + uri)) //nolint:gosec
250:            response := md5.Sum(b.Bytes()) //nolint:gosec

artifact/image/whiteout/whiteout.go
42:                     //nolint:nilerr // continue walking if there is an error

detector/list/list.go
71://nolint:gochecknoinits

extractor/filesystem/filesystem.go
241:    //nolint:containedctx

extractor/filesystem/list/list.go
229://nolint:gochecknoinits

extractor/standalone/list/list.go
69://nolint:gochecknoinits

@cuixq
Copy link
Collaborator Author

cuixq commented Feb 10, 2025

the two nolint in http_auth.go is actually commented out and other nolint is either just above a func/sturct or under - not sure if this is why. I cannot find the internal link to the rule that reports the space in a comment. we can try the fix that you proposed and see if it is accepted internally.

Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

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

Lets see if this works.

@cuixq cuixq changed the title chore: disable nolintlint chore: make nolint in its own line Feb 10, 2025
@cuixq
Copy link
Collaborator Author

cuixq commented Feb 10, 2025

good news - it is working internally!

@copybara-service copybara-service bot merged commit 7b7a37d into google:main Feb 10, 2025
8 checks passed
@cuixq cuixq deleted the lint branch February 10, 2025 02:29
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.

3 participants