3 ms·

New features are great, but I wish they'd make more of an investment in improving their core code review functionality. The lack of an "interdiff" view (between

New features are great, but I wish they'd make more of an investment in improving their core code review functionality. The lack of an "interdiff" view (between revisions of a PR) and the lack of a proper way to mark PRs as dependent on one another really limit the utility compared to other code review platforms (thinking specifically of Gerrit and Phabricator/Differential here).

chadlavi 7y ago
My god, would LOVE PR interdependence. Esp if I could leave a review like "approved after #123 is merged"
chatmasta 7y ago
You can compare any two commits. Maybe I’m misunderstanding, but isn’t that sufficient for you to see the diff of PR revisions? If the issue is reviewing the code, stale reviews for code that was pushed over will be marked as stale.
shadowfiend 7y ago
While it's a bit tucked away, when you are viewing the changes for a PR, there is a dropdown in the top left for “Changes from <all commits>”. You can select any range there to view changes from that range of commits. When you've left a review, you also generally get a “View Changes” button in the conversation that takes you to the changes since you last viewed, if new changes are pushed. The issue with this particular link is it tends to be ephemeral.
seer 7y ago
Github has a really great url api - you can usually do git related compares right there in the url - github.com/some/repo/compare/HEAD...comitsha is actually a thing. And while not particularly ergonomic you can have PR against other PR since forever. Github is built by devs for devs and has tons of hidden gems scattered in there. Do read their shortcuts help, blog and other dev sources and you might find tools for a lot of the task you can think of.
- Game_Ender 7y ago
  That feature breaks down in the presence of merges. But when those are not present it works OK, you see lines like this: > force-pushed the aojea:affinity branch from 8eab3f7 to ee626e7 yesterday [0] Where the "force pushed" links to a pretty decent UI [1], but it will include a bunch of non-import information if the person rebased and force pushed, which is a common reason for force pushing. You can see that in this [2] long lived Kubernetes PR that was rebased, the resulting "helpful" link shows me 2094 changed files [3]. In a tool like Gerrit or Phabricator this is automatically handled and it would basically ignore this operation unless the rebase changed the code being reviewed. 0 - https://github.com/kubernetes/kubernetes/pull/88409 https://github.com/kubernetes/kubernetes/pull/88409 1 - https://github.com/kubernetes/kubernetes/compare/8eab3f7316381a5bc91104881d9f77ee0430e56c..ee626e77bfd2d69dd378eade3d86a297f3bbab45 https://github.com/kubernetes/kubernetes/compare/8eab3f73163... 2 - https://github.com/kubernetes/kubernetes/pull/85000 https://github.com/kubernetes/kubernetes/pull/85000 3 - https://github.com/kubernetes/kubernetes/compare/375873cb532942035b9e6479f044a9184cf0e5a6..ee8f410ca145b028d352be2d9498fcbcf95cb517 https://github.com/kubernetes/kubernetes/compare/375873cb532...
Game_Ender 7y ago
According to the CEO's twitter account [0] they are release better support for dependent PRs this year, track progress of that feature here [1]. The crucial problem with code review on GitHub is that it's 100% dependent on Git to store the history of a PR. This too closely couples what a developer does locally on their machine to the code review process. This in turn means that force pushing drops the history of their "Checks" system, which annotate the PR with CI job an lint/test results. It also means junk commits on PRs make the whole review process more difficult. The app Reviewable [2] works with GitHub and fixes some of these issues if you want to give it a try. 0 - https://twitter.com/natfriedman/status/1170804894241972224 https://twitter.com/natfriedman/status/1170804894241972224 1 - https://github.com/isaacs/github/issues/959 https://github.com/isaacs/github/issues/959 2 - https://reviewable.io https://reviewable.io

chadlavi 7y ago

My god, would LOVE PR interdependence. Esp if I could leave a review like "approved after #123 is merged"

chatmasta 7y ago

You can compare any two commits. Maybe I’m misunderstanding, but isn’t that sufficient for you to see the diff of PR revisions? If the issue is reviewing the code, stale reviews for code that was pushed over will be marked as stale.

shadowfiend 7y ago

While it's a bit tucked away, when you are viewing the changes for a PR, there is a dropdown in the top left for “Changes from <all commits>”. You can select any range there to view changes from that range of commits. When you've left a review, you also generally get a “View Changes” button in the conversation that takes you to the changes since you last viewed, if new changes are pushed. The issue with this particular link is it tends to be ephemeral.

seer 7y ago

Github has a really great url api - you can usually do git related compares right there in the url - github.com/some/repo/compare/HEAD...comitsha is actually a thing. And while not particularly ergonomic you can have PR against other PR since forever. Github is built by devs for devs and has tons of hidden gems scattered in there. Do read their shortcuts help, blog and other dev sources and you might find tools for a lot of the task you can think of.

Game_Ender 7y ago
That feature breaks down in the presence of merges. But when those are not present it works OK, you see lines like this: > force-pushed the aojea:affinity branch from 8eab3f7 to ee626e7 yesterday [0] Where the "force pushed" links to a pretty decent UI [1], but it will include a bunch of non-import information if the person rebased and force pushed, which is a common reason for force pushing. You can see that in this [2] long lived Kubernetes PR that was rebased, the resulting "helpful" link shows me 2094 changed files [3]. In a tool like Gerrit or Phabricator this is automatically handled and it would basically ignore this operation unless the rebase changed the code being reviewed. 0 - https://github.com/kubernetes/kubernetes/pull/88409 https://github.com/kubernetes/kubernetes/pull/88409 1 - https://github.com/kubernetes/kubernetes/compare/8eab3f7316381a5bc91104881d9f77ee0430e56c..ee626e77bfd2d69dd378eade3d86a297f3bbab45 https://github.com/kubernetes/kubernetes/compare/8eab3f73163... 2 - https://github.com/kubernetes/kubernetes/pull/85000 https://github.com/kubernetes/kubernetes/pull/85000 3 - https://github.com/kubernetes/kubernetes/compare/375873cb532942035b9e6479f044a9184cf0e5a6..ee8f410ca145b028d352be2d9498fcbcf95cb517 https://github.com/kubernetes/kubernetes/compare/375873cb532...

Game_Ender 7y ago

According to the CEO's twitter account [0] they are release better support for dependent PRs this year, track progress of that feature here [1]. The crucial problem with code review on GitHub is that it's 100% dependent on Git to store the history of a PR. This too closely couples what a developer does locally on their machine to the code review process. This in turn means that force pushing drops the history of their "Checks" system, which annotate the PR with CI job an lint/test results. It also means junk commits on PRs make the whole review process more difficult. The app Reviewable [2] works with GitHub and fixes some of these issues if you want to give it a try. 0 - https://twitter.com/natfriedman/status/1170804894241972224 https://twitter.com/natfriedman/status/1170804894241972224 1 - https://github.com/isaacs/github/issues/959 https://github.com/isaacs/github/issues/959 2 - https://reviewable.io https://reviewable.io