-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: add support for ignore
and enforce
comment directives
#88
Conversation
fe5a8a7
to
18ef01c
Compare
) | ||
|
||
type Cache struct { | ||
comments map[*ast.File]ast.CommentMap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible optimizations I've realized when looking more into analysis framework: I believe analysis drivers like golangci-lint will never visit the same file more than once during an execution. So:
- it's not necessary to cache the comment map at all per file; it can be stored as a single variable in some pass-level visitor object
- there's no need to force atomic operations with mutexes in this context
More generally, it should be possible to limit the use of mutexes by using package-specific caches and only writing to the cache of the pass' own package (the package-to-cache lookup would still need to be synchronized due to concurrent package processing). There is no risk of concurrent read from and write to the same package cache because every reasonable driver will sequentially process packages in topological order.
Conclusion: it should only be necessary to do a single mutex lock when defining the package-specific cache in the cache lookup. After that, you can preserve the pointer to this package-specific cache in the visitor and use zero synchronization.
Unclear what perf impacts would be in practice. I might try this and see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- performance impact is very small.
- this cache is also expected to be used for definition directives, which are expected to be hit more than once during analysis. definition directiveas are yet to come, but still it is just future-proof design.
func HasDirective(comments []*ast.CommentGroup, expected Directive) bool { | ||
for _, cg := range comments { | ||
for _, commentLine := range cg.List { | ||
if strings.HasPrefix(commentLine.Text, string(expected)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small optimization: well-written directives should come after other comments (VSCode seems to reorder directives this way), so traversal of each comment group in reverse order would catch directives a bit faster.
Thanks for the inclusion @xobotyi. I greatly look forward to the release of this feature to golangci-lint. |
Hi @xobotyi , thanks for this feature - it seems super useful. I have a question regarding how this should work.
I've tried to solve a problem like this, but it seems like the commandline linter flags are taking precedence over the comments inside the files. If I provide an exclusion regex matching all structures, it ignores the structure with the 'enforce' directive. On the other hand, by default it will warn on all structures unless I explicitly request the structure to be ignored. Is this intended to work differently?
I can think of a workaround - to instead use |
@joestringer it seems you've missunderstood the place enforcement should happen - it should be used near the row structure usage happens. (i've read docs once again and it seems missleading to me now - will touch it up some time later) Thing you showing here is definition comments and unfortunately i did not manage to implement robust solution for these cases, as there is no builtin way to access definition's comment during analysis. |
Oh OK, thanks for the pointers. After a bit more thought, if I'm interested in specific patterns then I think it's better for me to just rely on the For reference I ended up with something like this in my
|
Allow defining
//exhaustruct:ignore
and//exhaustruct:enforce
comment for individual structure literals.//exhaustruct:ignore
allows to skip certain literals lint;//exhaustruct:enforce
allows to enable linting of certain literals, even if it was excluded by global config;Partial solution for #69 as it allows to ignore all structures with
.*
regex and enforce linting onyl in desired palces.Overlaps with #70, but is simpler an more atomic solution for the case. Described PR will be used as a base to bring in the explicit mode and keep @navneethjayendran contribution.
Readme intentionally left untouched as it will be rewritten altogether.