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

Fix #808 #986

Merged
merged 3 commits into from
May 10, 2024
Merged

Fix #808 #986

merged 3 commits into from
May 10, 2024

Conversation

chavacava
Copy link
Collaborator

Closes #808 by ignoring comments matching the official pattern for directives

@chavacava chavacava requested review from mgechev and denisvmedia May 10, 2024 09:42
Copy link
Collaborator

@denisvmedia denisvmedia left a comment

Choose a reason for hiding this comment

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

LGTM

@chavacava chavacava merged commit 85333f8 into master May 10, 2024
4 checks passed
@denisvmedia denisvmedia deleted the fix-808 branch May 10, 2024 10:46
@ccoVeille
Copy link
Contributor

For reference, here is what is implemented in gofumpt

https://github.com/mvdan/gofumpt/blob/master/format%2Fformat.go#L334

They use a slightly different regexp

You can look at historical changes, it gives context

@@ -165,3 +165,9 @@ func checkNumberOfArguments(expected int, args lint.Arguments, ruleName string)
panic(fmt.Sprintf("not enough arguments for %s rule, expected %d, got %d. Please check the rule's documentation", ruleName, expected, len(args)))
}
}

var directiveCommentRE = regexp.MustCompile("//(line |extern |export |[a-z0-9]+:[a-z0-9])") // see https://go-review.googlesource.com/c/website/+/442516/1..2/_content/doc/comment.md#494

Choose a reason for hiding this comment

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

I wonder if it needs to be anchored (^); I did a quick try using the comment below, and it didn't flag that as invalid;

//this is a regular command that's incorrectly formatted //nolint:foobar // because one two three

(disclaimer: I only used the function directly, not in context of the whole code, so perhaps there's already some other code handling this)

Copy link
Contributor

@ccoVeille ccoVeille May 11, 2024

Choose a reason for hiding this comment

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

I think you are right, that's why I raised the point

#986 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

It was fixed with #988

Choose a reason for hiding this comment

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

Thanks!

@ccoVeille
Copy link
Contributor

Please note this change had a unexpected (but normal) consequence for me

Previously the following syntax were allowed

//nolint: somelinter // what ever the reason

This one was somehow invalid, and excluded by the code before this PR

not it reports a normal error, because it expects no space between comment delimiter and comment text

//nolint:somelinter // what ever the reason

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.

comment-spacings false positives for nolint directives
4 participants