Extractor: give the go.mod comment groups a source location #232
Extractor: give the go.mod comment groups a source location #232
Conversation
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.
Looks sensible overall, some minor comments.
import go | ||
|
||
from CommentGroup cg | ||
select cg.getLocation() |
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.
The test runner automatically includes the location in the test output, so in general you shouldn't need to explicitly select locations in tests.
extractor/gomodextractor.go
Outdated
extractGoModComment(tw, comment, grouplbl, idx) | ||
idx++ | ||
commentEndCol := comment.Start.LineRune + len(comment.Token) |
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.
Our end positions are generally inclusive, so I think this should be
commentEndCol := comment.Start.LineRune + len(comment.Token) | |
commentEndCol := comment.Start.LineRune + len(comment.Token) - 1 |
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.
Also, do I interpret the test failures on Windows correctly to mean that comment.Token
includes a \r
if the line ending is \r\n
? If so, we'll probably want to chop it off.
8cd41b5
to
90b06c2
Compare
The comment group is now omitted entirely if empty, and otherwise delimits the range of the comments ascribed to this group.
90b06c2
to
ce417aa
Compare
The |
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.
One minor suggestion, otherwise LGTM. (And well done for reporting the bug!)
extractor/gomodextractor.go
Outdated
} | ||
} | ||
|
||
func stripTrailingCR(input string) string { |
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.
Maybe just use https://golang.org/pkg/strings/#TrimSuffix?
This means the source locations and lengths for comments will be the same on Windows and Unix-like platforms. Note this works around golang/go#39913
ce417aa
to
0252c72
Compare
@max-schaefer done |
Makefile improvements
The comment group is now omitted entirely if empty, and otherwise delimits the range of the comments ascribed to this group.
Pre-emptive reviewer response: yes, really, Go's
math
package only providesMin
andMax
for float types, so you either have to cast back and forth, take a dependency such asgithub.com/pkg/math
or write your ownIntMin
andIntMax
.