Resolves #70: add stricter comment directive scan #72
Conversation
| // We have "if" instead of "else if" here in case of conflicting ignore/enforce directives. | ||
| // In that case, because this is second, we will default to enforcing. | ||
| requireDefaultCase = true | ||
| } | ||
|
|
||
| if sw.Tag == nil { | ||
| return true, resultNoSwitchTag // never possible for valid Go program? |
There was a problem hiding this comment.
It is possible, e.g.
switch {
case err == nil:
return result, nil
case errors.Is(err, NotFound):
return result, nil
default:
return nil, err
}
| requireDefaultCase = false | ||
| } | ||
| if directivesIncludes(uDirectives, enforceDefaultCaseRequiredComment) { | ||
| if uDirectives.hasDirective(enforceDefaultCaseRequiredDirective) { | ||
| // We have "if" instead of "else if" here in case of conflicting ignore/enforce directives. |
There was a problem hiding this comment.
I wonder whether it would make more sense for the analyzer to raise an error in this case. I don't foresee it happening much in practice though.
I'm more inclined to believe typos like //exhaustive:enfocre should result in errors.
There was a problem hiding this comment.
I wonder whether it would make more sense for the analyzer to raise an error in this case. I don't foresee it happening much in practice though.
If you're referring to the scenario in which valid ignore and enforce comments are both present on the same switch statement — yes, reporting an error is preferable. Related issue
I'm more inclined to believe typos like //exhaustive:enfocre should result in errors.
I agree that reporting an error for typoed and unknown directives (e.g. "enfocre", "enforce-none") is preferable.
| requireDefaultCase = false | ||
| } | ||
| if directivesIncludes(uDirectives, enforceDefaultCaseRequiredComment) { | ||
| if uDirectives.hasDirective(enforceDefaultCaseRequiredDirective) { | ||
| // We have "if" instead of "else if" here in case of conflicting ignore/enforce directives. |
There was a problem hiding this comment.
I wonder whether it would make more sense for the analyzer to raise an error in this case. I don't foresee it happening much in practice though.
If you're referring to the scenario in which valid ignore and enforce comments are both present on the same switch statement — yes, reporting an error is preferable. Related issue
I'm more inclined to believe typos like //exhaustive:enfocre should result in errors.
I agree that reporting an error for typoed and unknown directives (e.g. "enfocre", "enforce-none") is preferable.
comment.go
Outdated
|
|
||
| type directiveSet int64 | ||
|
|
||
| func parseDirectiveSet(commentGroups []*ast.CommentGroup) (out directiveSet) { |
There was a problem hiding this comment.
Minor: Rename to parseDirectives. The "set" portion of the function name is a detail about the return type that is not necessary here.
comment.go
Outdated
| return | ||
| } | ||
|
|
||
| func (d directiveSet) hasDirective(directive directive) bool { |
There was a problem hiding this comment.
Minor: Shorten the method name to has. The shorter name is sufficiently clear, because checking for the presence of a directive is the apparent operation on this type.
It also avoids stutter in call sites.
directives.hasDirective(enforceDirective)
vs.
directives.has(enforceDirective)
comment.go
Outdated
|
|
||
| type directiveSet int64 | ||
|
|
||
| func parseDirectiveSet(commentGroups []*ast.CommentGroup) (out directiveSet) { |
There was a problem hiding this comment.
| func parseDirectiveSet(commentGroups []*ast.CommentGroup) (out directiveSet) { | |
| func parseDirectiveSet(commentGroups []*ast.CommentGroup) directiveSet { |
Minor: Remove the name for the return parameter in the function declaration. The name does not add value here.
There was a problem hiding this comment.
Mostly to avoid the var out directiveSet declaration but fair enough
|
Thanks for the change. It looks good to me overall. I added a few comments. |
|
@nishanths Thanks for the feedback, addressed all. |
|
I added another test and a small change. |
|
Thank you! |
Add
directiveSetbitset data structure anddirectiveenum with stricter parsing to not recognize comments like//exhaustive:enforce-noneas valid directives. Also return errors on invalid directives or combinations thereof.Resolves #70, #49