Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

More code review tools

github.com

Post reply on HN
Print view View original

111–120 of 151 posts

Re: More code review tools

#111

Post by scoot »

cdcarter wrote: post #104
gmartres wrote: post #93

Earlier quoted context omitted.

> Its something we're still working on, but it still helps to vocalize your support for wanting improved force push support. :D YES! I wish for that feature everyday. Gerrit does it exactly right. Related to this, I really, really hope that you will consider displaying commits in a PR in the correct order, suppose that I have three commits on top of master: a->b->c->master If I git rebase -i master and reorder my com…

Seriously -- disappearing comments is very sad.

Disappearing comments mean they've been adressed. And they don't actually disappear, they're just minimised.

Re: More code review tools

#112

Post by scoot »

joshpeek wrote: post #67
js2 wrote: post #21

It would be nice if GitHub supported a Gerrit-inspired code-review process, where instead of having to choose between: 1) piling new commits onto the existing branch/PR, or 2) force-pushing and completely losing the old commits on the server You could instead push into a "magic" ref-spec and the server would retain the original commits, but also the rewritten commits, such that a PR can have multiple revisions. This…

Hey, I'm one of the devs for this new PR stuff. This flow is something we're hoping to improve as well. Force push support is pretty second class right now, any previous commits and discussion basically gets lost. It sucks. So we already use a internal refspec for tracking the latest PR HEAD. You can actually manually fetch this under `refs/pull/123/head`. However, this is only the latest HEAD, not any previous histo…

It's your workflow that's broken. Add commits, then squash before merge when the review is done.

Re: More code review tools

#113

Post by forgotpwtomain »

I can't say I'm a fan. If I select a single commit, instead of giving me a list with a single commit check-marked, it gives me only that single commit and an option to 'show all commits'. So to change the commit you are viewing requires clicking 'show all commits' (waiting for them to load) and then selecting the other commit you want to view (which should just be a check box).

Also it totally baffles me that these actions all require server-side requests. It's really a lot easier to go to the old commits tab, and to ctrl click an open tab for each commit then to use this new feature.

At one point Github offered the best and cleanest UI of all the alternatives - but I doubt this will be the case for long at this rate.

Re: More code review tools

#114

Post by forgotpwtomain »

I can't say I'm a fan. If I select a single commit, instead of giving me a list with a single commit check-marked, it gives me only that single commit and an option to 'show all commits'. So to change the commit you are viewing requires clicking 'show all commits' and then selecting the other commit you want to view.

Also it totally baffles me that these are all server-side actions. Github for a long-time had the cleanest and most effective UI - but at this rate I doubt it will stay that way long..

Re: More code review tools

#115

Post by js2 »

bradfitz wrote: post #95
joshpeek wrote: post #67

Earlier quoted context omitted.

Hey, I'm one of the devs for this new PR stuff. This flow is something we're hoping to improve as well. Force push support is pretty second class right now, any previous commits and discussion basically gets lost. It sucks. So we already use a internal refspec for tracking the latest PR HEAD. You can actually manually fetch this under `refs/pull/123/head`. However, this is only the latest HEAD, not any previous histo…

Btw, I've been gluing Github PRs and Gerrit together with https://github.com/LetsUseGerrit Example in action: https://github.com/grpc/grpc-go/pull/570

I also wrote a Gerrit to GitHub PR proxy... I should ask $dayjob about open-sourcing it. And I seem to recall there's another as well implemented as a gerrit plugin.

Re: More code review tools

#116

Post by jdavis703 »

Pxtl wrote: post #72
mdo wrote: post #50

Earlier quoted context omitted.

Code review tooling has been in the works for a long time. It's sadly taking us this long to actually ship the first fundamental bits, but there's a ton that this unlocks for us. Expect to see more soon :). (Source: I worked on code review at GitHub for awhile!)

In that case, is there any plans for your processes to change to get a more rapid release-cycle? Going dark for N months to implement a feature seems to fly in the face of the workflow GitHub tends to inspire.

Sometimes you're on a rapid release cycle making non-user facing changes that will allow you to release new user-facing features. I have no idea if this is what was going on at Github, but I've been at plenty of places where the code and/or infrastructure was so messed up, seemingly trivial updates would take weeks and months to complete.

Re: More code review tools

#117

Post by wpietri »

hwangmoretime wrote: post #13

Code review is something all of us do, but all of us do differently. Anyone know of any nice frameworks, articles, or blog posts for code review? I'm particularly interested in the case where knowledge transfer is a high priority in the code review. My current side project integrates feedback theory [1], to provide scaffolds and other cues to help and remind reviewers to give high quality feedback. Thus, my interest.…

For knowledge transfer, my number one technique is pair programming. If there are more than two people involved, I add frequent pair rotation.

The only time I've ever felt comfortable going on vacation is on pair programming teams. That's a sign to me that knowledge really has been properly transferred.

Re: More code review tools

#118

Post by stormbrew »

Only related to review in a somewhat indirect way, but I really wish the commit view on github (as well as other git tools) had an equivalent of --left-only command line option to git-log. It's incredibly useful for viewing a high level of the history of a repo that uses merge bubbles, and lack of tooling around doing just that seems like that main reason people do things like squash their branches before merging to master (which is where I think it connects back to review).

Re: More code review tools

#119

Post by gmartres »

scoot wrote: post #112
joshpeek wrote: post #67

Earlier quoted context omitted.

Hey, I'm one of the devs for this new PR stuff. This flow is something we're hoping to improve as well. Force push support is pretty second class right now, any previous commits and discussion basically gets lost. It sucks. So we already use a internal refspec for tracking the latest PR HEAD. You can actually manually fetch this under `refs/pull/123/head`. However, this is only the latest HEAD, not any previous histo…

It's your workflow that's broken. Add commits, then squash before merge when the review is done.

Part of a good review should be to review how you decided to split your PR into separate commits (does every commit have a single purpose, or did you sneak unrelated changes in a commit? Could the commit message be improved?). Furthermore, sometimes you have to rebase your PR on top of master (for example, because things changed in master too much, or because your PR depends on a patch that was recently accepted in master to actually work), this shouldn't make review comments disappear.

Re: More code review tools

#120

Post by steveklabnik »

scoot wrote: post #111
cdcarter wrote: post #104

Earlier quoted context omitted.

Seriously -- disappearing comments is very sad.

Disappearing comments mean they've been adressed. And they don't actually disappear, they're just minimised.

Yes, this is my largest pain points with PRs: I'd like to be able to collapse comments that were resolved, but weren't made on a line that changed.
Post reply on HN