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

Fix #808 #986

Merged
merged 3 commits into from
May 10, 2024
Merged

Fix #808 #986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec97779
fix #808 by using the official regexp for directives
May 10, 2024
cea51ee
more precise regexp and testcases
May 10, 2024
18e4b6c
factors out the detection of directive comments to use it in other rules
May 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions rule/comment-spacings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,7 @@ func (r *CommentSpacingsRule) configure(arguments lint.Arguments) {
defer r.Unlock()

if r.allowList == nil {
r.allowList = []string{
"//go:",
"//revive:",
"//nolint:",
}

r.allowList = []string{}
for _, arg := range arguments {
allow, ok := arg.(string) // Alt. non panicking version
if !ok {
Expand Down Expand Up @@ -87,5 +82,5 @@ func (r *CommentSpacingsRule) isAllowed(line string) bool {
}
}

return false
return isDirectiveComment(line)
}
6 changes: 6 additions & 0 deletions rule/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

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 •
edited
Loading

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

Copy link

Choose a reason for hiding this comment

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

Thanks!


func isDirectiveComment(line string) bool {
return directiveCommentRE.MatchString(line)
}
5 changes: 5 additions & 0 deletions testdata/comment-spacings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,8 @@ type c struct {
//+optional
d *int `json:"d,omitempty"`
}

//extern open
//export MyFunction

//nolint:gochecknoglobals
Loading