diff --git a/cmd/api-linter/github_actions.go b/cmd/api-linter/github_actions.go index e43ed93d9..55efdd665 100644 --- a/cmd/api-linter/github_actions.go +++ b/cmd/api-linter/github_actions.go @@ -33,22 +33,8 @@ func formatGitHubActionOutput(responses []lint.Response) []byte { fmt.Fprintf(&buf, "::error file=%s", response.FilePath) if problem.Location != nil { - // Some findings are *line level* and only have start positions but no - // starting column. Construct a switch fallthrough to emit as many of - // the location indicators are included. - switch len(problem.Location.Span) { - case 4: - fmt.Fprintf(&buf, ",endColumn=%d", problem.Location.Span[3]) - fallthrough - case 3: - fmt.Fprintf(&buf, ",endLine=%d", problem.Location.Span[2]) - fallthrough - case 2: - fmt.Fprintf(&buf, ",col=%d", problem.Location.Span[1]) - fallthrough - case 1: - fmt.Fprintf(&buf, ",line=%d", problem.Location.Span[0]) - } + location := lint.FileLocationFromPBLocation(problem.Location, nil) + fmt.Fprintf(&buf, ",line=%d,col=%d,endLine=%d,endColumn=%d", location.Start.Line, location.Start.Column, location.End.Line, location.End.Column) } // GitHub uses :: as control characters (which are also used to delimit diff --git a/cmd/api-linter/github_actions_test.go b/cmd/api-linter/github_actions_test.go index fd87cc958..e51e8ad66 100644 --- a/cmd/api-linter/github_actions_test.go +++ b/cmd/api-linter/github_actions_test.go @@ -53,27 +53,11 @@ func TestFormatGitHubActionOutput(t *testing.T) { Span: []int32{5, 6, 7}, }, }, - { - RuleID: "line::col", - Message: "Line and column", - Location: &descriptorpb.SourceCodeInfo_Location{ - Span: []int32{5, 6}, - }, - }, - { - RuleID: "line", - Message: "Line only", - Location: &descriptorpb.SourceCodeInfo_Location{ - Span: []int32{5}, - }, - }, }, }, }, - want: `::error file=example.proto,endColumn=8,endLine=7,col=6,line=5,title=line։։col։։endLine։։endColumn::line, column, endline, and endColumn -::error file=example.proto,endLine=7,col=6,line=5,title=line։։col։։endLine::Line, column, and endline -::error file=example.proto,col=6,line=5,title=line։։col::Line and column -::error file=example.proto,line=5,title=line::Line only + want: `::error file=example.proto,line=6,col=7,endLine=8,endColumn=8,title=line։։col։։endLine։։endColumn::line, column, endline, and endColumn +::error file=example.proto,line=6,col=7,endLine=6,endColumn=7,title=line։։col։։endLine::Line, column, and endline `, }, { @@ -98,8 +82,8 @@ func TestFormatGitHubActionOutput(t *testing.T) { }, }, }, - want: `::error file=example.proto,endColumn=4,endLine=3,col=2,line=1,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names -::error file=example.proto,endColumn=8,endLine=7,col=6,line=5,title=core։։naming_formats։։field_names::multi\nline\ncomment\n\nhttps://linter.aip.dev/naming_formats/field_names + want: `::error file=example.proto,line=2,col=3,endLine=4,endColumn=4,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names +::error file=example.proto,line=6,col=7,endLine=8,endColumn=8,title=core։։naming_formats։։field_names::multi\nline\ncomment\n\nhttps://linter.aip.dev/naming_formats/field_names `, }, { diff --git a/lint/problem.go b/lint/problem.go index cbadd2232..98bb8d945 100644 --- a/lint/problem.go +++ b/lint/problem.go @@ -87,14 +87,14 @@ func (p Problem) marshal() interface{} { return struct { Message string `json:"message" yaml:"message"` Suggestion string `json:"suggestion,omitempty" yaml:"suggestion,omitempty"` - Location fileLocation `json:"location" yaml:"location"` + Location FileLocation `json:"location" yaml:"location"` RuleID RuleName `json:"rule_id" yaml:"rule_id"` RuleDocURI string `json:"rule_doc_uri" yaml:"rule_doc_uri"` Category string `json:"category,omitempty" yaml:"category,omitempty"` }{ p.Message, p.Suggestion, - fileLocationFromPBLocation(loc, p.Descriptor), + FileLocationFromPBLocation(loc, p.Descriptor), p.RuleID, p.GetRuleURI(), p.category, @@ -106,35 +106,35 @@ func (p Problem) GetRuleURI() string { return getRuleURL(string(p.RuleID), ruleURLMappings) } -// position describes a one-based position in a source code file. +// Position describes a one-based Position in a source code file. // They are one-indexed, as a human counts lines or columns. -type position struct { +type Position struct { Line int `json:"line_number" yaml:"line_number"` Column int `json:"column_number" yaml:"column_number"` } -// fileLocation describes a location in a source code file. +// FileLocation describes a location in a source code file. // // Note: Positions are one-indexed, as a human counts lines or columns // in a file. -type fileLocation struct { - Start position `json:"start_position" yaml:"start_position"` - End position `json:"end_position" yaml:"end_position"` +type FileLocation struct { + Start Position `json:"start_position" yaml:"start_position"` + End Position `json:"end_position" yaml:"end_position"` Path string `json:"path" yaml:"path"` } -// fileLocationFromPBLocation returns a new fileLocation object based on a +// FileLocationFromPBLocation returns a new fileLocation object based on a // protocol buffer SourceCodeInfo_Location -func fileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d desc.Descriptor) fileLocation { +func FileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d desc.Descriptor) FileLocation { // Spans are guaranteed by protobuf to have either three or four ints. span := []int32{0, 0, 1} if l != nil { span = l.Span } - var fl fileLocation + var fl FileLocation if d != nil { - fl = fileLocation{Path: d.GetFile().GetName()} + fl = FileLocation{Path: d.GetFile().GetName()} } // If `span` has four ints; they correspond to @@ -143,11 +143,11 @@ func fileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d desc.Descripto // We add one because spans are zero-indexed, but not to the end column // because we want the ending position to be inclusive and not exclusive. if len(span) == 4 { - fl.Start = position{ + fl.Start = Position{ Line: int(span[0]) + 1, Column: int(span[1]) + 1, } - fl.End = position{ + fl.End = Position{ Line: int(span[2]) + 1, Column: int(span[3]), } @@ -159,11 +159,11 @@ func fileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d desc.Descripto // // We add one because spans are zero-indexed, but not to the end column // because we want the ending position to be inclusive and not exclusive. - fl.Start = position{ + fl.Start = Position{ Line: int(span[0]) + 1, Column: int(span[1]) + 1, } - fl.End = position{ + fl.End = Position{ Line: int(span[0]) + 1, Column: int(span[2]), } diff --git a/lint/problem_test.go b/lint/problem_test.go index 96424c57f..45e1bee9f 100644 --- a/lint/problem_test.go +++ b/lint/problem_test.go @@ -120,3 +120,37 @@ func TestProblemDescriptor(t *testing.T) { }) } } + +func TestFileLocationFromPBLocation(t *testing.T) { + tests := []struct { + testName string + pbLocation *dpb.SourceCodeInfo_Location + expectedLocation FileLocation + }{ + { + testName: "MultilineSpan", + pbLocation: &dpb.SourceCodeInfo_Location{Span: []int32{2, 0, 5, 70}}, + expectedLocation: FileLocation{ + Start: Position{3, 1}, + End: Position{6, 70}, + }, + }, + { + testName: "SingleLineSpan", + pbLocation: &dpb.SourceCodeInfo_Location{Span: []int32{2, 0, 42}}, + expectedLocation: FileLocation{ + Start: Position{3, 1}, + End: Position{3, 42}, + }, + }, + } + + for _, test := range tests { + t.Run(test.testName, func(t *testing.T) { + actual := FileLocationFromPBLocation(test.pbLocation) + if actual != test.expectedLocation { + t.Errorf("Got:\n%v\nExpected\n%v", actual, test.expectedLocation) + } + }) + } +}