handle some block comment to detect generated files #1161
Conversation
| for scanner.Scan() { | ||
| line := strings.TrimSpace(scanner.Text()) | ||
| if strings.HasPrefix(line, "//") { | ||
| if inBlockComment { |
There was a problem hiding this comment.
sorry, I don't think it's a good idea to manually parse comments.
Maybe we can use ast.ParseFile with flag like PackageClauseOnly or similar to stop early? I'm sure that performance won't be an issue here if we implement it properly.
There was a problem hiding this comment.
Thank you for your advice.
I was actually on the fence about using AST, but I'll try to do that.
There was a problem hiding this comment.
I've changed to stop parsing manually by using parser.ParseFile().
Although parser.ParseFile() reads entire file regardless of mode, with PackageClauseOnly mode (and ParseComments mode), it stops parsing files after import statements, as you pointed out.
There was a problem hiding this comment.
The text may appear anywhere in the file.
Are we sure that avoiding possible performance regression has higher priority that compliance with convention?
There was a problem hiding this comment.
I agree that it looks more important. I guess time still will be <100ms total. We can easily check it: golangci-lint run -v shows all processors' times.
There was a problem hiding this comment.
Do you mean compliance with convention is more important?
Suppose you think so.. performance regression depends on how many issues will be reported on generated files, so I think it's difficult to choose which repository to measure for this purpose. But as you explain in usage, parsing file seems not so much cost.
Parsing one source file takes 200 us on average. Parsing of all files in $GOROOT/src takes 2 seconds. Currently we parse each file more than once because it's not the bottleneck.
However, you also write like this:
We're planning to parse each file only once.
If we check entire file for comments, should we use existing AST instead of parsing file here again?
I haven't confirmed if it's possible yet, I guess we can do so since this function is called on reporting phase.
|
|
||
| // Issue 954: Some lines can be very long, e.g. auto-generated | ||
| // embedded resources. Reported on file of 86.2KB. | ||
| func TestGetDocFileWithLongLine(t *testing.T) { |
There was a problem hiding this comment.
This change replaces the whole implementation of getDoc(), so I also added a test for confirming previous issue (#954).
I changed autogenerated_exclude.go to handle block comments partially to detect generated comment inside or after the block comments.
I avoided to change to read whole file, because it might affect to performance.
Although the generated comment text may appear anywhere in the file, this change still only see around the top of the file.
It will quit reading when the line is other than comments, package statement, and empty.
I'm not sure it can be accepted because it's not a perfect solution, but I think it will be improved than it is now.
Related: #1083