search

google / git-appraise

Distributed code review system for Git repos
Apache License 2.0
5.12k stars 145 forks source link

Implement full source file range support #83

Closed tcolgate closed 6 years ago

tcolgate commented 6 years ago

The review format from ShipeShape supports fulle specification of a block of text withint a file. We implement this by allowing the cli to take a range specified as:

startLine[+startColumn][:endLine[+endColumn]]

We need to fully check the semantics of the specified location. Lines and columns are number from 1 -> max(uint32). The start must be before the start of the file.

ojarjur commented 6 years ago

Thanks for the contribution!

I've only had a chance to glance over the change so far, but it looks good. In particular, thanks for fixing the preexisting issues with the import orderings.

I'll put together more detailed feedback early next week.

ojarjur commented 6 years ago

Sorry that it took me a while to get more detailed feedback on this, but overall I really like it.

In particular, you are right that line numbers should be treated as starting at 1; I like to think of the line number representing the number of lines that should be displayed before showing the comment, so a comment that should be displayed for the first line would have a start line of "1", while a comment with a start line of "0" should be displayed before any of the file contents.

I also really like how you managed to make the flag for the new functionality consistent with the current behavior; someone wanting to just comment on a whole like can still do that with the exact same command, while someone wanting to specify a comment for a precise range has a concise way to do that.

I'll leave more detailed feedback on the relevant lines of code; but overall I like it.

Thanks for the contribution!

tcolgate commented 6 years ago

@ojarjur I've pushed some updates. Also updated the appraise show output to display the full range if it has been specified

ojarjur commented 6 years ago

Thanks, @tcolgate. This change looks great.

Since the title starts with [WIP], are there other things you wanted to get in before we merge this?

tcolgate commented 6 years ago

The WIP was because (if I'm completely honest), I hadn't actually run the code at the point that I raised the original PR.

ojarjur commented 6 years ago

Thanks again for taking the time and putting in the effort to help improve the tool.

This is a great addition.

tcolgate commented 6 years ago

Thanks for the review. git-appraise is looking really good. I'm going to focus a bit on seeing if I can get a reasonable vim integration working (there are some projects up and running, but there's not much progress yet).