[GHC DevOps Group] [fwd] GitHub code review infrastructure

Sun Nov 26 13:42:04 UTC 2017

Hi Ben,

Just some quick workarounds:

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?

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.

Both are not ideal solutions but might alleviate those issues.

Best,
Jonas

On Wed, 22 Nov 2017, 19:56 Ben Gamari <ben at well-typed.com> wrote:

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