Sitelet https://web.archive.org/web/20200529202937/https://github.com/cli/cli/pull/935
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

patch review prototype #935

Open
wants to merge 11 commits into
base: trunk
from
Open

patch review prototype #935

wants to merge 11 commits into from

Conversation

@vilmibm
Copy link
Member

vilmibm commented May 15, 2020 •

NARRATED VIDEO DEMO: https://www.youtube.com/watch?v=LFeuKxolMUg

This PR is a prototype of a "patch mode" approach to reviewing. You can gh pr checkout this and
run it with gh pr review -p 123. It will never try to post an actual review.

What functions in the prototype:

  • making comments
  • making an overall review decision
  • skipping a change
  • skipping a file

what is mocked but not functioning:

  • saving in progress review
  • displaying a diff on demand

what is not mocked but part of the idea:

  • suggestions
  • resuming an in progress review
  • canceling an in progress review

I'm curious if people have any feedback, up to and including whether or not this is worth fleshing
out more and putting on a roadmap.

(It hopefully goes without saying that the code is a nightmare and very little of it should ever actually be merged)

vilmibm added 11 commits May 12, 2020
hmm
@vilmibm vilmibm added the prototype label May 15, 2020
@probablycorey
Copy link
Collaborator

probablycorey commented May 18, 2020

Thanks for the video @vilmibm, it was great to hear your thoughts while seeing the prototype in action. This works better than I expected it to! Below are some thoughts I had while watching this:

  • What would view diff do. Since I'm already looking at a diff I was confused
  • I like how you can see the diff in the discarded section while you are commenting.
  • I would feel some hesitation to use this because it isn't easy to toggle between the different diff hunks. Most of the time I move up and down the code a bunch before leaving a review. Making this easy and obvious would make this feel less like I have one shot to do it correctly.
@mislav
Copy link
Member

mislav commented May 25, 2020

Loved the video demo! 💯

I can see that this was modelled after git add --patch, one of my most often-used tools. So naturally, I would also be biased towards supporting this style of interaction.

What I find missing from this concept is a fleshed-out way for commenting on a specific line (or a set of lines). Furthermore, I think that a code review tool should allow adding comments specifically on either added lines (right side of split diff) or removed lines (left side of split diff). From your prototype, it looks like it's only possible to comment on whole chunks of diff. Did you have a vision on how to enable more precise targeting for comments?

I second all @probablycorey's points; I think you did explain what is the use-case for v: view diff in your narration, but without that explanation it was also not clear to me what would "view diff" do.

Overall, I feel that this style of interaction is promising and I'd be interested in adding it to our roadmap.

@mislav mislav changed the base branch from master to trunk May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.