Show inline github comments inline

[danvk]

For example, on this review, kberg left a comment on a particular line using the github UI. I'd like to see it on the same line in reviewable.

I realize github stores line notes in a somewhat odd format. But I've implemented the mapping before, so I know it's possible! And this would greatly facilitate moving back and forth between github and reviewable.

[pkaminski]

Hey Dan, you're right that this would be useful and that the line mapping problem is annoying but solvable. There are other issues though that have prevented me from doing this, though:

1. Reviewable comments are threaded, so you can have distinct discussions on the same line. AFAICT GitHub comments have no way to recover such an association, so I'd need to glom all the comments for a line into one discussion.
2. If I mapped GitHub line comments into Reviewable line comments, users would expect me to map them in the other direction as well. However, AFAICT it's only possible to create GitHub line comments one at a time, so I'd have to drop the "single email for entire comment batch" feature, which I see as core to Reviewable.
3. Mixing GitHub and Reviewable on a single PR wouldn't be a great experience even if the line comments were mapped, since the tracking of reviewed files and replied / resolved comments wouldn't work right.

You've clearly given code reviews on GitHub a lot of thought (I wish I'd found better-pull-requests before I started on this project!) so I'd value any further ideas you have on this.

[danvk]

Reviewable comments are threaded, so you can have distinct discussions on the same line.

One thought here is that you could embed additional information (e.g. thread IDs) in the links to Reviewable. But you'd lose information for any comments coming from github. You'd probably have to thread them in a best-effort way. I don't think this would be too big a deal -- most review comment "threads" for me consist of two messages: "Please do this", "Done".

... users would expect me to map them in the other direction as well ...

Yep, reading but not writing inline comments would be awkward. It's interesting that you see the batched emails as core. I think it's a nice feature, but the more important thing to me is that I receive all the emails at once. That way "receiving email" == "reviewer is done", an indication that has to come through some other communications channel with regular pull requests.

Mixing GitHub and Reviewable on a single PR wouldn't be a great experience

For sure it wouldn't be an ideal experience, but it would make it much easier for teams to transition from pure-GitHub PRs to Reviewable. Thinking about my team, it's unlikely that I'll convince everyone overnight to switch to Reviewable. But if I can start using it while everyone else is on GitHub, maybe I can slowly get them to move over.

This was the idea with gitcritic aka better-pull-requests. Take the GitHub PR model and build a nicer UI on top of it. The only state I had was draft comments, which I would send out all at once when you wanted to publish. The differentiating points were: 1) side-by-side diffs (which github didn't have at the time) 2) draft/batched comments 3) see diffs within the PR.

I wound up abandoning the effort for a few reasons. I was showing diffs between commits in the PR, but I realized that the "snapshot"/"revision" model would be better. It looks like you've done that right. I also learned that others were working on similar products (like Reviewable!) and that GitHub was also focusing on improving pull requests. My strategy now is to improve the GitHub PR UI using a Chrome Extension.

[pkaminski]

It's interesting that you see the batched emails as core.

In the interviews I did with developers prior to designing Reviewable about half felt strongly about this.

Thinking about my team, it's unlikely that I'll convince everyone overnight to switch to Reviewable.

I'm curious, are you talking about a team on an open-source project, or a corporate team?

I can certainly see the value to getting traction in smoothing the transition from GitHub to Reviewable (@JoshRosen had a similar request), but the downside is that as the integration gets tighter Reviewable's value added goes down. I'm still trying to figure out how to strike the right balance... What would you think about having an explicit switch to put a review into "GitHub compatibility mode", where line comments would be mapped bidirectionally, you'd be limited to one discussion per line, and discussion resolution tracking would be disabled? Would it be OK to trigger such a mode manually, or should it be automatic based on whether review participants are registered with Reviewable, or perhaps even set on a per-repo basis?

I also learned that others were working on similar products (like Reviewable!) and that GitHub was also focusing on improving pull requests.

Any leads you have on other tools and efforts would be most welcome. Among recent GitHub-specific products, I know about Review Ninja, Sourcegraph, CodeReviewHub, and GitColony, and I'm not aware of any focus on PRs at GitHub.

[danvk]

I do think a "compatibility mode" would help ease the transition for my team, but you probably have a better sense of the value of that to the broader population of developers based on your interviews. My team works on open source projects, but we all sit in the same office.

The only other tool I know of is Indifference, which I don't believe has any public demo yet.

I had some direct correspondence with mdo, who worked on the split diffs feature for github PRs. He said "We’ve been putting a lot of effort into our code review functionality and that’ll start showing more in the coming months. We’ve got a whole bucket list of things to do that, like split diffs, we should’ve done years ago." I asked about viewing diffs within the progression of a PR and batched comments. He said those were both "on our radar as well".

[pkaminski]

Thanks for the info @danvk, I've reached out to mdo to see if he's willing to chat.

After thinking about this a bit and getting feedback from some other people, I've decided that it's worth mapping GitHub inline comments to inline comments in Reviewable to smooth the transition into the tool, so I'll be implementing that soon. However, I won't be mapping inline comments back to GitHub to support a mixed worfklow -- I think my time is better spent on adding more awesome feature to Reviewable, so that people will be able to convince their whole team (or at least their reviewers/reviewees) to switch outright. 😄

[pkaminski]

I picked this up again and, after some back-and-forth with GitHub support, came to the conclusion that it's not possible to reliably map GitHub inline comments into Reviewable. The core of the issue is that the data for an inline comment doesn't provide the sha of the base commit, so it's not possible to figure out what blobs the diff was between. The PR's base diff cannot be used instead as it may have changed since the comment was made -- especially in the common case where Reviewable is backfilling a review that's been in progress for some time.

Marking as wontfix unless/until GitHub improves its API.

[pkaminski]

I now do best-effort mapping of line comments from GitHub: if the comment is on a base or positive delta line, and if Reviewable has captured the target commit as a revision and can find the right file. Line comments that fail to be mapped correctly are added to the top-level discussion as before, but now with a warning that points to this issue.

[danvk]

I just used this feature for the first time this week—I have to say, it's fantastic. It's really nice to be able to switch over to Reviewable once a review that you expected to be simple becomes large.

Thanks for implementing it! You might want to remove the "wontfix" label :)

[pkaminski]

Glad to hear it worked for you @danvk, but in actuality a lot of stars have to align just right for GitHub line comments to be ported successfully. So a more accurate label would be "fixed as much as possible, but it ain't perfect and not gonna get better" -- I'll settle for "wontfix". 😃

[tomprince]

I now do best-effort mapping of line comments from GitHub: if the comment is on a base or positive delta line, and if Reviewable has captured the target commit as a revision and can find the right file.

I'm not sure how connecting repositories work, but it would be nice if comments made after a repository is connected were always mapped correctly (perhaps by forcing a revision). I haven't managed to convince all my team to switch to reviewable yet and it would be nice to be able to take over a review where there have been several rounds of review in github and finish them off in reviewable, where I can examine the changes that have come from the various review comments.

[pkaminski]

Hey @tomprince, as it turns out Reviewable already does everything in its power to make comments mappable: a GitHub comment makes the previous commit a "desired revision point", and will nearly always cause a revision to be created for that commit. (The exception being if this would result in the creation of too many revisions at once, with the limit set at 10 by default.) So pretty much the only comments that won't be mapped correctly once a repo has been connected are the ones on negative delta lines, and there GitHub simply doesn't provide sufficient information to reliably deduce the correct target (!).

If you see any mis-mapped comments outside of those constraints, please open an issue since you're probably seeing a bug. Thanks!

[iphydf]

Perhaps this can be revisited now that github has threaded review comments as well.

[pkaminski]

Definitely, but there's no API available yet. I'm tracking this in #418.