<p dir="ltr">Hi Ben,</p>
<p dir="ltr">Just some quick workarounds:</p>
<p dir="ltr">It's possible to click on a line of code and paste it into a comment, that could act as a replacement for your pointers?</p>
<p dir="ltr">In regards to diff changes, try adding ?ws=1 to the URL to get a diff without the whitespace, in some cases it might help.</p>
<p dir="ltr">Both are not ideal solutions but might alleviate those issues.</p>
<p dir="ltr">Best,<br>
Jonas</p>
<br><div class="gmail_quote"><div dir="ltr">On Wed, 22 Nov 2017, 19:56 Ben Gamari <<a href="mailto:ben@well-typed.com">ben@well-typed.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
Attached is a message which I wrote to Manuel shortly after ICFP<br>
relating some recent experiences with GitHub's code review<br>
functionality.<br>
<br>
Since I wrote this there are a few additional issues that I stumbled<br>
upon write reviewing other patches which I would like to mention,<br>
<br>
* It is not possible to comment on lines that are not in the vicinity of<br>
a change. I often find myself doing this to point out parts of the<br>
code that a contributor may have overlooked.<br>
<br>
* I would reiterate the difficulty of keeping ones bearings in a large<br>
patch. Recently I reviewed Moritz's Relocatable builds patch [1]<br>
and found myself frequently wondering what file I was looking at.<br>
While in Differential I can simply cast a glance to the file tree, in<br>
GitHub I need to scroll up until I spot the beginning of the file,<br>
and then find where I was previously and resume.<br>
<br>
* I have found that GitHub is quite conservative in pointing out<br>
similarities between old and new code which tends to obscure what<br>
should be obvious preserved structure. To take a recent example: see<br>
GitHub's rendering [2] of a piece of Alan's recent TTG work and try<br>
to work out what that hunk is doing. Now look at the same hunk on<br>
Phabricator [3]. The difference is in my opinion far more obvious.<br>
<br>
All of these papercuts add up and have a real cost. If the consensus<br>
really is that we want to move review to GitHub then I will certainly go<br>
along, but I do feel that it would be a significant regression.<br>
<br>
I have still not yet tried using Reviewable.io, so I can't say with<br>
certainty whether it would address these concerns.<br>
<br>
Cheers,<br>
<br>
- Ben<br>
<br>
<br>
[1] <a href="https://github.com/snowleopard/hadrian/pull/445/files" rel="noreferrer" target="_blank">https://github.com/snowleopard/hadrian/pull/445/files</a><br>
[2] <a href="https://github.com/ghc/ghc/commit/47ad6578ea460999b53eb4293c3a3b3017a56d65#diff-df7abd11782680fc55c1d1b04747dba9R1103" rel="noreferrer" target="_blank">https://github.com/ghc/ghc/commit/47ad6578ea460999b53eb4293c3a3b3017a56d65#diff-df7abd11782680fc55c1d1b04747dba9R1103</a><br>
[3] <a href="https://phabricator.haskell.org/rGHC47ad6578ea460999b53eb4293c3a3b3017a56d65#C8856OL84" rel="noreferrer" target="_blank">https://phabricator.haskell.org/rGHC47ad6578ea460999b53eb4293c3a3b3017a56d65#C8856OL84</a><br>
<br>
<br>
-------------------- Start of forwarded message --------------------<br>
From: Ben Gamari <<a href="mailto:ben@smart-cactus.org" target="_blank">ben@smart-cactus.org</a>><br>
To: Manuel Chakravarty <<a href="mailto:manuel.chakravarty@tweag.io" target="_blank">manuel.chakravarty@tweag.io</a>><br>
Subject: GitHub code review infrastructure<br>
BCC: <a href="mailto:ben@smart-cactus.org" target="_blank">ben@smart-cactus.org</a><br>
Date: Thu, 28 Sep 2017 09:20:20 -0400<br>
Message-ID: <<a href="mailto:87y3ozrmcr.fsf@ben-laptop.smart-cactus.org" target="_blank">87y3ozrmcr.fsf@ben-laptop.smart-cactus.org</a>><br>
<br>
Hi Manuel,<br>
<br>
Since we spoke I've been working with a contributor who submitted a<br>
somewhat non-trivial change [1] via pull request. This was my first<br>
non-trivial code review with on GitHub with myself in the role of<br>
reviewer. By GHC standards the patch is neither large nor subtle; it's<br>
largely just me guiding a new contributor through the codebase.<br>
<br>
I'll admit that after this experience I'm a bit less rosy on GitHub's<br>
code review functionality than I was on Monday. I thought it might be<br>
helpful (for me, if not also for you) to summarize my impressions:<br>
<br>
* Commenting works well although quoting bits of comments is a bit<br>
inconvenient.<br>
<br>
* The lack of an overview of the diff (like Differential's "file tree"<br>
pane) makes reviewing patches of moderate-to-large size rather tricky<br>
<br>
* The treatment of rebased branches is truly awful. The previous<br>
commits appear to be totally gone and cannot be viewed. This is<br>
in contrast to Phabricator, where you can view each revision of<br>
a differential, which in the past has been very helpful when we<br>
are trying to reconstruct the reasoning behind a change.<br>
<br>
* While the discussion on pre-rebase commits is preserved, it is<br>
impossible to view it in the context in which it was made since the<br>
patches themselves are gone.<br>
<br>
* The "Conversation" interface gets rather hard to follow after a short<br>
while since there are free-standing comments, new commit<br>
notifications, and inline code comments in a low-contrast,<br>
low-whitespace wall of boxes.<br>
<br>
Some of these issues above may be just a function of my unfamiliarity<br>
with the interface. Many of the other issues could no doubt be worked<br>
around with a combination of Greasemonkey and custom CSS. That being<br>
said, the fact that this may be necessary is a bit unfortunate and the<br>
more fundamental issues (inability to view previously revisions,<br>
treatment of rebases) can't be worked around.<br>
<br>
This is a shame since, as I related on Monday, I think the arcanist<br>
workflow introduces a lot of friction. Relying on Git, a tool nearly all<br>
our users are familiar with, for patch management makes much more sense<br>
IMHO. However, I'll admit I was hoping that GitHub's code review<br>
experience was better than it is.<br>
<br>
Perhaps <a href="http://reviewable.io" rel="noreferrer" target="_blank">reviewable.io</a> or some alternative would be better?<br>
<br>
Anyways, these are just my two-pence. I'm still happy to consider moving<br>
and am not ruling out that on the whole it still may be the right thing<br>
to do; however, I was just a bit surprised by some of the issues that I<br>
encountered and thought it would only be fair to let you know.<br>
<br>
Cheers,<br>
<br>
- Ben<br>
<br>
<br>
[1] <a href="https://github.com/ghc/ghc/pull/76/files" rel="noreferrer" target="_blank">https://github.com/ghc/ghc/pull/76/files</a><br>
-------------------- End of forwarded message --------------------<br>
_______________________________________________<br>
Ghc-devops-group mailing list<br>
<a href="mailto:Ghc-devops-group@haskell.org" target="_blank">Ghc-devops-group@haskell.org</a><br>
<a href="https://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devops-group" rel="noreferrer" target="_blank">https://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devops-group</a><br>
</blockquote></div>