-
Notifications
You must be signed in to change notification settings - Fork 27
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
Exclude generated code from coverage #23
base: master
Are you sure you want to change the base?
Conversation
scanner/scanner.go
Outdated
if err != nil { | ||
return errors.WithStack(err) | ||
} | ||
if !doNotEditRe.Match(body) { |
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.
hmm, maybe it would be good to make the check a bit more thorough, similar to the isGenerated
from the lint
tool?
just to be a bit more strict / thorough and not exclude some frankenstein file with a comment floating around somewhere mid-file?
https://pkg.go.dev/cmd/go#hdr-Generate_Go_files_by_processing_source specifies:
This line must appear before the first non-comment, non-blank text in the file.
you can see the test case as well from here which things should be valid: https://github.com/golang/lint/blob/85993ffd0a6cd043291f3f63d45d656d97b165bd/lint_test.go#L292
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.
@rubensayshi You are right and I am aware of this specification (I linked it myself in hexdigest/gowrap#20). My issue is, that one of my main use-cases is exactly with gowrap
and this tool generates a valid comment, but does not (yet) comply with strict requirement for the position, because the comment is inserted after the package
statement.
If you agree, I can change the detection, such that it complies with the test set you linked, which would be sufficient for me.
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.
@rubensayshi I added the isGenerated
function you linked to in a separate commit.
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.
ehm, I suppose it's fine to adjust isGenerated
to also accept package
before the comment as an exception?
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.
This is already the case in the "original" version of isGenerated
, see:
courtney/scanner/generated_test.go
Line 25 in 0e4312f
{"package foo\n// Code generated by some tool. DO NOT EDIT.\ntype foo int\n", true}, |
I assume, this because this code predates the decision in golang/go#41196
9f95252
to
2747b62
Compare
Should there be a flag/config option for this? There could be projects interested in coverage of generated code. |
Yeah I think running tests on generated code is common... I do it in some of my projects. |
@dave No problem, I can add a flag for this. Do you have any preference on the name of the flag? I assume, the default would be to keep the existing behavior for backwards compatibility reasons. |
2747b62
to
0e4312f
Compare
Fixes #15