6 ms·

I've heard this point of view many times, but cannot find an extensive explanation of it. Could anyone elaborate on the issue with the GitHub review UI/UX? > I

I've heard this point of view many times, but cannot find an extensive explanation of it. Could anyone elaborate on the issue with the GitHub review UI/UX? > I hate the GitHub review UI with a passion. At least, right now, GitHub PRs are not a viable option for Mozilla [...] the more general shortcomings in the review UI.

steveklabnik 3y ago
I know people who express similar feelings. Usually it is shorthand for "I would prefer stacked diffs" or similar. Two blog posts I've seen people point at: * https://mitchellh.com/writing/github-changesets https://mitchellh.com/writing/github-changesets * https://jg.gg/2018/09/29/stacked-diffs-versus-pull-requests/ https://jg.gg/2018/09/29/stacked-diffs-versus-pull-requests/
- anyonecancode 3y ago
  From the second article, a minor point but possibly helpful to other here, he contrasts doing everything in the terminal with stacked commits vs going to the Github UI. If people aren't aware, Github offers a cli tool[1]. I've been using it for a few months now and am finding it does make me more productive -- it's nice to be able to open up a PR directly from my terminal. I do still use the GH UI for a lot of things, but I'll often at least start in the terminal, and it also makes the transition from terminal to browser easy as many commands support the `--web` flag open up the right page for you (eg `gh repo view --web`). [1] https://cli.github.com/ https://cli.github.com/
  - lima 3y ago
    The CLI doesn't help with stacked commits, though. There's tools like spr[1] but none of them are anywhere as pleasant to use as Gerrit (or Phabricator, I guess). [1]: https://github.com/ejoffe/spr https://github.com/ejoffe/spr
    - anyonecancode 3y ago
      It doesn't -- hence my noting the GH cli as a minor point. It's a tool I've personally found helped increase my productivity, so just wanted to share as others may find it useful too.
- shepherdjerred 3y ago
  I've found that Graphite [0] has solved this issue for me. It's makes the experience of shipping stacked diffs about the same as the CR tool within AWS [0]: https://graphite.dev/ https://graphite.dev/
  - steveklabnik 3y ago
    Thanks for this. I admittedly still don't really grok it, but maybe if I play around with the tooling, something will click.
    - shepherdjerred 3y ago
      I think that this page gives a decent overview of what the tool aims to accomplish: https://graphite.dev/docs/how-stacking-works https://graphite.dev/docs/how-stacking-works This page covers some of the more advanced functionality: https://graphite.dev/docs/manage-stacks https://graphite.dev/docs/manage-stacks Either way, I hope you like it! I've tried many other tools that aim to implement stacked diffs for GitHub PRs, and Graphite is _by far_ the best. They have their own Web UI but I skip that completely and just use their CLI + GH's UI.
      - steveklabnik 3y ago
        I watched their product overview video last night, but like, I don't grok what the difference between rebasing a branch and rebasing a stack is, and they said something like "if there are conflicts, this is when you'd fix them." but I thought the point was reducing conflicts. It sounds to me like short lived feature branches with a lot of extra terminology. I really think I just need to actually try the tooling and then examine the git repo. I am very bottom-up brained when it comes to git, and so I need to visualize what commands actually do to the DAG to really grok what's going on.
- deleted 3y ago
  [deleted]
- globular-toast 3y ago
  What are these people talking about? What's the difference between "stacked diffs" and just adding commits to a branch? Who cares if the branch is called "master" or not? You could also force push updated commits (e.g. after fixup and rebase etc) and tools like Gitlab and GitHub can show you the diff between versions (or "changesets", as they call them). Gitlab at least makes it pretty clear when a change has been made and the order things were done in. Pretty sure GitHub does too.
  - steveklabnik 3y ago
    As I alluded to, it’s their opinion, not mine. I don’t fully understand the differences myself. But they’re very passionate about this.
whoateallthepy 3y ago
One point: if you are re-reviewing, other platforms (e.g. Phabricator, Gerrit) have much more developed ways to compare changes relative to one another.
- KerrAvon 3y ago
  Phabricator was awful. It loaded commit messages with irrelevant boilerplate and allowed people to post patches that you couldn’t build for testing because they were just diffs and not actual branches in the repo. Not sad to see it go.
sylvestre 3y ago
See https://news.ycombinator.com/item?id=37836932 https://news.ycombinator.com/item?id=37836932 by a Mozilla employee
ge96 3y ago
bitbucket is worse, reviewees can resolve reviewer's comments (github has this too turns out), after file changes, comments disappear
- AlotOfReading 3y ago
  What's wrong with reviewees marking comments resolved? That allows anyone reading to see what issues are "open" or already addressed at a glance. Code review isn't adversarial, so it's not like the reviewee is closing comments to shove bad code through and they need approvals anyway.
  - ge96 3y ago
    It's when they resolve it without verifying if their solution was good
  - DoctorDabadedoo 3y ago
    Not bad if you have a junior that knows they are a junior, bad otherwise. My pet peeve with bitbucket was that when doing code review, you couldn't send all comments and overall review in a batch, every single comment was received in real time by the author. Sometimes I would be still reviewing code when the author pushed fixes to comments I've made before and that still might need other changes. After 15 years they improved that 2 weeks ago. I'm glad it's better, but I hate it with a burning passion.
- lmm 3y ago
  Bitbucket is much better IME. It has a better handle on the current state of the review rather than offering you the whole diff to re-review whenever someone rebases, and it loses comments much less than github (indeed not at all, in my experience).
dllthomas 3y ago
One issue I was discussing with a colleague just the other day is that you cannot leave a comment on lines that are not "part of the diff" (changed or context), which is sometimes really useful for "see this thing over here".
- mattrick 3y ago
  You definitely can comment on lines that haven't been changed. I don't think there's a way to comment on files that haven't been changed, though.
  - dllthomas 3y ago
    As the other commenter pointed out, only if they're close enough to a changed line. That's what I meant by "context" (borrowing terminology from diff, where you change the amount of it with -C).
  - codeapprove 3y ago
    Actually you can’t comment on lines that haven’t been changed if they’re too far from the changed lines! It’s a weird limitation of GitHubs data model. (source: that’s one of the features that people like most about the review tool I created, CodeApprove)
    - dllthomas 3y ago
      Annotations can be added by "checks" anywhere, though, I believe. I like to use that to surface cross-references (annotating anchors whose references appear in changed or context lines, and references whose anchors do).
zoogeny 3y ago
I once worked with a guy who hated Python. He also hated IntelliJ and did all of his programming in Eclipse. He hated MacOS and would only run old versions of Windows of Linux. What I found was that he was just sensitive to change. It took him awhile to get comfortable with something but as soon as he did he would from then on resist any change. Anything that was different to what he was used to he hated. In some sense, it makes sense. People should prefer processes over tools in many cases. If you have a process that works with one tool but doesn't work with a new tool then you might be better off sticking with the original tool. Broken processes can debilitate organizations. When people say "I hate X" often what they are really saying is "I have a process to do Y that works for me but tool X makes that process too difficult". The problem is they just say "I hate X" so you never really hear about the process Y which provides the context.
- lima 3y ago
  GitHub PRs are both bad process (no stacking or per-commit review, incentivizes large changes, ...) and bad tool (no inter-diffs, noisy diffs, comments disappear through rebases, slow, ...).
  - KerrAvon 3y ago
    > slow but have you tried bitbucket? Omfg is it slow
- deleted 3y ago
  [deleted]
- naasking 3y ago
  > Anything that was different to what he was used to he hated. I suspect he just hated the constant churn. That's kind of a rational frame of mind when you think about it.
  - erik_seaberg 3y ago
    I enjoy learning new things, but my bar for "is this actually better?" seems to be unconsciously higher than it used to be.
jeffbee 3y ago
Github review is good for either zero or one round of comments. After that you can't tell what the hell is going on. It has clearly been written by people who do not themselves practice code review. Compare and contrast with Gerrit, which remains usable after many rounds of comments, in both directions, and changes. Compare also Phabricator.
aseipp 3y ago
Most of the projects that dislike GitHub's review UI want the functional equivalent of `git range-diff`. Code review systems like Phabricator and Gerrit basically revolve around this as basis of thinking about diffs, their evolution, and how code review progresses. You want to write 3 patches to a project, that are committed in series, based off of `X` A ---> B ---> C X---/ Let's say A cleans up some code, getting it ready for B; it is not mandatory but was just naturally done as part of the change; maybe it's 50 lines. B then adds 500 lines of new code to be reviewed. Finally, C integrates the new code from B; maybe you migrate something to use it, changing a few internal API calls. So C might be a diff of only 5 or 10 lines. The code reviewer will want to start by reading A, and leaving comments on A. And so on for B, and so on for C. Let's say they leave code review comments on each commit. So now you have a set of things to do. GitHub encourages you to add a new set of patches on top of the previous 3, so you might publish add commits on top: A ---> B ---> C --- > D ---> E ---> ... X---/ Where each change after C incrementally addresses review comments. In contrast, tools like Gerrit and Phabricator say instead that you should publish new versions of A, B, and C, wholesale. So now we have a new series of 3 patches: A' ---> B' ---> C' X---/ This branch might exist in parallel to your old one. So your full commit graph might look like this, now: A ---> B ---> C / /-A' ---> B' ---> C' X---/ So there's the original series of changes A,B,C, and the new series of changes that respond to all the comments. Think of that as "version 1" of the series and then "version 2" Now here's the question: how does the code reviewer know that you addressed their comments? Answer: they need to do a range diff between the original version 1 series and the version 2 series: A ---> B ---> C | | | d(A,A') d(B,B') d(C,C') | | | A' ---> B' ---> C' Where d(x,y) = diff(x,y). You're looking at the diff between the two versions of one patch. So instead you view the changes between version 1 of A, and version 2 of A. And so on and so forth for all 3 patches. This is very useful for example because B might be 500 lines, but responding to review comments may only take 50 lines of fixes. It would be very annoying to re-read the entire 500 line patch, as opposed to just the 50 line incremental patch. This has very big effects as the review cycle goes forth. People mostly like this review style because it keeps needless "fix review comment" commits out of the history and it "localizes" the unit of code review to each individual patch rather than the whole aggregate. Note that the final version of the series A,B,C will just contain those 3 logical changes, not 3 logical changes + 1 dozen fixup changes. This not only makes changes more "dense", it improves the ability to navigate the history, and do things like `git blame`; and it means you don't commit things like "fix failing test" which would break bisection. Note that most of the systems that implement the above review style do not literally use `git range-diff` in their implementation; rather, git range-diff is simply an implementation of this idea that you should review each version of a patch as a diff from the previous version. The tools themselves have their own lifecycle, patch management, APIs, etc that are wholly different from Git's. Finally, there are lots of things GitHub's UX is just slacking on, functionality aside. You can't comment "anywhere" in a code review, just on changed lines. An annoying one I hate is that you can batch review comments but not batch review resolutions; if you leave 5 comments on a diff, you submit them as a batch. But if you resolve 5 comments, you do it one at a time, which is annoying and easy to lose track of. The UX has too many tabs you have to swap between, which is pointless when you could just make the page a little longer and things like Ctrl+F would work better. And so on, and so forth.
codeapprove 3y ago
Bias warning: I created CodeApprove to give GitHub reviews a better UI. GitHub PR review UI is … fine. But it has very little depth. Unlike an IDE it’s hard to grow into a power user of it. It seems to be optimized for simple reviews with 1-2 rounds of a few comments each. When you do dozens of code reviews every week you want something more. You want something snappy and dense with keyboard shortcuts. You want something that draws your attention to where it’s needed and something that helps you ensure all your conversations reach resolution. So there’s a lot of room for improvement and GitHub hasn’t shown much interest in improving this area. So 3p UIs are stepping in (Graphite, CodeApprove, Reviewable, etc)

steveklabnik 3y ago

I know people who express similar feelings. Usually it is shorthand for "I would prefer stacked diffs" or similar. Two blog posts I've seen people point at: * https://mitchellh.com/writing/github-changesets https://mitchellh.com/writing/github-changesets * https://jg.gg/2018/09/29/stacked-diffs-versus-pull-requests/ https://jg.gg/2018/09/29/stacked-diffs-versus-pull-requests/

anyonecancode 3y ago
From the second article, a minor point but possibly helpful to other here, he contrasts doing everything in the terminal with stacked commits vs going to the Github UI. If people aren't aware, Github offers a cli tool[1]. I've been using it for a few months now and am finding it does make me more productive -- it's nice to be able to open up a PR directly from my terminal. I do still use the GH UI for a lot of things, but I'll often at least start in the terminal, and it also makes the transition from terminal to browser easy as many commands support the `--web` flag open up the right page for you (eg `gh repo view --web`). [1] https://cli.github.com/ https://cli.github.com/
- lima 3y ago
  The CLI doesn't help with stacked commits, though. There's tools like spr[1] but none of them are anywhere as pleasant to use as Gerrit (or Phabricator, I guess). [1]: https://github.com/ejoffe/spr https://github.com/ejoffe/spr
  - anyonecancode 3y ago
    It doesn't -- hence my noting the GH cli as a minor point. It's a tool I've personally found helped increase my productivity, so just wanted to share as others may find it useful too.
shepherdjerred 3y ago
I've found that Graphite [0] has solved this issue for me. It's makes the experience of shipping stacked diffs about the same as the CR tool within AWS [0]: https://graphite.dev/ https://graphite.dev/
- steveklabnik 3y ago
  Thanks for this. I admittedly still don't really grok it, but maybe if I play around with the tooling, something will click.
  - shepherdjerred 3y ago
    I think that this page gives a decent overview of what the tool aims to accomplish: https://graphite.dev/docs/how-stacking-works https://graphite.dev/docs/how-stacking-works This page covers some of the more advanced functionality: https://graphite.dev/docs/manage-stacks https://graphite.dev/docs/manage-stacks Either way, I hope you like it! I've tried many other tools that aim to implement stacked diffs for GitHub PRs, and Graphite is _by far_ the best. They have their own Web UI but I skip that completely and just use their CLI + GH's UI.
    - steveklabnik 3y ago
      I watched their product overview video last night, but like, I don't grok what the difference between rebasing a branch and rebasing a stack is, and they said something like "if there are conflicts, this is when you'd fix them." but I thought the point was reducing conflicts. It sounds to me like short lived feature branches with a lot of extra terminology. I really think I just need to actually try the tooling and then examine the git repo. I am very bottom-up brained when it comes to git, and so I need to visualize what commands actually do to the DAG to really grok what's going on.
deleted 3y ago
[deleted]
globular-toast 3y ago
What are these people talking about? What's the difference between "stacked diffs" and just adding commits to a branch? Who cares if the branch is called "master" or not? You could also force push updated commits (e.g. after fixup and rebase etc) and tools like Gitlab and GitHub can show you the diff between versions (or "changesets", as they call them). Gitlab at least makes it pretty clear when a change has been made and the order things were done in. Pretty sure GitHub does too.
- steveklabnik 3y ago
  As I alluded to, it’s their opinion, not mine. I don’t fully understand the differences myself. But they’re very passionate about this.

whoateallthepy 3y ago

One point: if you are re-reviewing, other platforms (e.g. Phabricator, Gerrit) have much more developed ways to compare changes relative to one another.

KerrAvon 3y ago
Phabricator was awful. It loaded commit messages with irrelevant boilerplate and allowed people to post patches that you couldn’t build for testing because they were just diffs and not actual branches in the repo. Not sad to see it go.

sylvestre 3y ago

See https://news.ycombinator.com/item?id=37836932 https://news.ycombinator.com/item?id=37836932 by a Mozilla employee

ge96 3y ago

bitbucket is worse, reviewees can resolve reviewer's comments (github has this too turns out), after file changes, comments disappear

AlotOfReading 3y ago
What's wrong with reviewees marking comments resolved? That allows anyone reading to see what issues are "open" or already addressed at a glance. Code review isn't adversarial, so it's not like the reviewee is closing comments to shove bad code through and they need approvals anyway.
- ge96 3y ago
  It's when they resolve it without verifying if their solution was good
- DoctorDabadedoo 3y ago
  Not bad if you have a junior that knows they are a junior, bad otherwise. My pet peeve with bitbucket was that when doing code review, you couldn't send all comments and overall review in a batch, every single comment was received in real time by the author. Sometimes I would be still reviewing code when the author pushed fixes to comments I've made before and that still might need other changes. After 15 years they improved that 2 weeks ago. I'm glad it's better, but I hate it with a burning passion.
lmm 3y ago
Bitbucket is much better IME. It has a better handle on the current state of the review rather than offering you the whole diff to re-review whenever someone rebases, and it loses comments much less than github (indeed not at all, in my experience).

dllthomas 3y ago

One issue I was discussing with a colleague just the other day is that you cannot leave a comment on lines that are not "part of the diff" (changed or context), which is sometimes really useful for "see this thing over here".

mattrick 3y ago
You definitely can comment on lines that haven't been changed. I don't think there's a way to comment on files that haven't been changed, though.
- dllthomas 3y ago
  As the other commenter pointed out, only if they're close enough to a changed line. That's what I meant by "context" (borrowing terminology from diff, where you change the amount of it with -C).
- codeapprove 3y ago
  Actually you can’t comment on lines that haven’t been changed if they’re too far from the changed lines! It’s a weird limitation of GitHubs data model. (source: that’s one of the features that people like most about the review tool I created, CodeApprove)
  - dllthomas 3y ago
    Annotations can be added by "checks" anywhere, though, I believe. I like to use that to surface cross-references (annotating anchors whose references appear in changed or context lines, and references whose anchors do).

zoogeny 3y ago

I once worked with a guy who hated Python. He also hated IntelliJ and did all of his programming in Eclipse. He hated MacOS and would only run old versions of Windows of Linux. What I found was that he was just sensitive to change. It took him awhile to get comfortable with something but as soon as he did he would from then on resist any change. Anything that was different to what he was used to he hated. In some sense, it makes sense. People should prefer processes over tools in many cases. If you have a process that works with one tool but doesn't work with a new tool then you might be better off sticking with the original tool. Broken processes can debilitate organizations. When people say "I hate X" often what they are really saying is "I have a process to do Y that works for me but tool X makes that process too difficult". The problem is they just say "I hate X" so you never really hear about the process Y which provides the context.

lima 3y ago
GitHub PRs are both bad process (no stacking or per-commit review, incentivizes large changes, ...) and bad tool (no inter-diffs, noisy diffs, comments disappear through rebases, slow, ...).
- KerrAvon 3y ago
  > slow but have you tried bitbucket? Omfg is it slow
deleted 3y ago
[deleted]
naasking 3y ago
> Anything that was different to what he was used to he hated. I suspect he just hated the constant churn. That's kind of a rational frame of mind when you think about it.
- erik_seaberg 3y ago
  I enjoy learning new things, but my bar for "is this actually better?" seems to be unconsciously higher than it used to be.

jeffbee 3y ago

Github review is good for either zero or one round of comments. After that you can't tell what the hell is going on. It has clearly been written by people who do not themselves practice code review. Compare and contrast with Gerrit, which remains usable after many rounds of comments, in both directions, and changes. Compare also Phabricator.

aseipp 3y ago

Most of the projects that dislike GitHub's review UI want the functional equivalent of `git range-diff`. Code review systems like Phabricator and Gerrit basically revolve around this as basis of thinking about diffs, their evolution, and how code review progresses. You want to write 3 patches to a project, that are committed in series, based off of `X` A ---> B ---> C X---/ Let's say A cleans up some code, getting it ready for B; it is not mandatory but was just naturally done as part of the change; maybe it's 50 lines. B then adds 500 lines of new code to be reviewed. Finally, C integrates the new code from B; maybe you migrate something to use it, changing a few internal API calls. So C might be a diff of only 5 or 10 lines. The code reviewer will want to start by reading A, and leaving comments on A. And so on for B, and so on for C. Let's say they leave code review comments on each commit. So now you have a set of things to do. GitHub encourages you to add a new set of patches on top of the previous 3, so you might publish add commits on top: A ---> B ---> C --- > D ---> E ---> ... X---/ Where each change after C incrementally addresses review comments. In contrast, tools like Gerrit and Phabricator say instead that you should publish new versions of A, B, and C, wholesale. So now we have a new series of 3 patches: A' ---> B' ---> C' X---/ This branch might exist in parallel to your old one. So your full commit graph might look like this, now: A ---> B ---> C / /-A' ---> B' ---> C' X---/ So there's the original series of changes A,B,C, and the new series of changes that respond to all the comments. Think of that as "version 1" of the series and then "version 2" Now here's the question: how does the code reviewer know that you addressed their comments? Answer: they need to do a range diff between the original version 1 series and the version 2 series: A ---> B ---> C | | | d(A,A') d(B,B') d(C,C') | | | A' ---> B' ---> C' Where d(x,y) = diff(x,y). You're looking at the diff between the two versions of one patch. So instead you view the changes between version 1 of A, and version 2 of A. And so on and so forth for all 3 patches. This is very useful for example because B might be 500 lines, but responding to review comments may only take 50 lines of fixes. It would be very annoying to re-read the entire 500 line patch, as opposed to just the 50 line incremental patch. This has very big effects as the review cycle goes forth. People mostly like this review style because it keeps needless "fix review comment" commits out of the history and it "localizes" the unit of code review to each individual patch rather than the whole aggregate. Note that the final version of the series A,B,C will just contain those 3 logical changes, not 3 logical changes + 1 dozen fixup changes. This not only makes changes more "dense", it improves the ability to navigate the history, and do things like `git blame`; and it means you don't commit things like "fix failing test" which would break bisection. Note that most of the systems that implement the above review style do not literally use `git range-diff` in their implementation; rather, git range-diff is simply an implementation of this idea that you should review each version of a patch as a diff from the previous version. The tools themselves have their own lifecycle, patch management, APIs, etc that are wholly different from Git's. Finally, there are lots of things GitHub's UX is just slacking on, functionality aside. You can't comment "anywhere" in a code review, just on changed lines. An annoying one I hate is that you can batch review comments but not batch review resolutions; if you leave 5 comments on a diff, you submit them as a batch. But if you resolve 5 comments, you do it one at a time, which is annoying and easy to lose track of. The UX has too many tabs you have to swap between, which is pointless when you could just make the page a little longer and things like Ctrl+F would work better. And so on, and so forth.

codeapprove 3y ago

Bias warning: I created CodeApprove to give GitHub reviews a better UI. GitHub PR review UI is … fine. But it has very little depth. Unlike an IDE it’s hard to grow into a power user of it. It seems to be optimized for simple reviews with 1-2 rounds of a few comments each. When you do dozens of code reviews every week you want something more. You want something snappy and dense with keyboard shortcuts. You want something that draws your attention to where it’s needed and something that helps you ensure all your conversations reach resolution. So there’s a lot of room for improvement and GitHub hasn’t shown much interest in improving this area. So 3p UIs are stepping in (Graphite, CodeApprove, Reviewable, etc)