fix: supply proper location for GitHub Action format #1081
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
noahdietz
changed the title
|
cc @achew22 author of the original change for |
|
CLA signed and check re-run. Definitely open to whatever feedback here. Like I mentioned on #1071, there's a more-focused path that only fixes the |
noahdietz
changed the title
As a fellow OSS maintainer, I share your 😅 . Thanks for this tool, it's hugely helpful! |
| fmt.Fprintf(&buf, ",line=%d", problem.Location.Span[0]) | ||
| } | ||
| location := lint.FileLocationFromPBLocation(problem.Location) | ||
| fmt.Fprintf(&buf, ",line=%d,col=%d,endLine=%d,endColumn=%d", location.Start.Line, location.Start.Column, location.End.Line, location.End.Column) |
There was a problem hiding this comment.
Why do we believe that all findings contain line and column numbers? Sorry if that's obvious from other changes in the project but this caused it to nil pointer exception for me before I made the change you're removing here
There was a problem hiding this comment.
This definitely depends on the logic in lint/problem.go being correct, but according to the code comment on line 128 the underlying Span should be guaranteed to have either 3 (line,startCol,endCol) or 4 (startLine,startCol,endLine,endCol) values.
I'd be curious if you have a case to reproduce that, and if it also causes problems for the YAML and JSON output? I think those should panic on main if a span is ever too short
lint/problem.go
Outdated
| // protocol buffer SourceCodeInfo_Location | ||
| func fileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location) fileLocation { | ||
| func FileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location) FileLocation { |
There was a problem hiding this comment.
I realize now that this is more or less directly reversing a deliberate decision in #195 to not have two types of "location." If that's still desirable then maybe the other path of fixing the math directly in the github formatter, or possibly a third option of trying to centralize the "formatter" concept somewhere they could share utilities like this
Make the internal method lint.FileLocationFromPBLocation public so that formatters beyond the YAML and JSON marshal methods can rely on the same logic to translate protobuf Spans into useable file locations.
Use the lint.FileLocationFromPBLocation method to get the correct problem locations for the GitHub Actions formatter, rather than re-implementing the conversion from PB location to file location.
jsocol
force-pushed
the
fix/github-actions-formatter
branch
from
b018fd9
to
5a64cb4
Compare
This fixes the issue in #1071 with the GitHub Action output using the wrong location values. It's in two commits. The first exposes
lint.FileLocationFromPBLocationas a public method, and adds tests for it. The second uses that method to remove the conversion logic from the GitHub Action formatter and fixes the tests that had the file locations specified.