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

81–90 of 151 posts

Re: More code review tools

#81

Post by danpalmer »

davegaeddert wrote: post #75
danpalmer wrote: post #26

Earlier quoted context omitted.

Unfortunately that doesn't work for us for 2 reasons - 1. There's a data loss bug when multiple people edit the PR descriptions. I've had that as an open issue with GitHub for ~6 months, my team experiences it several times a week. 2. We use the checkboxes to assign and track reviewers, and since there's only one count of checkboxes, it would mess with our "2 of 5 complete" kind of metric for reviews. I'd like first-…

You might be interested in taking a look at PullApprove ( https://pullapprove.com/ ) -- basically you put a YAML in your repo that defines what code review looks like for your team (who, when, how many need to approve, etc. -- http://docs.pullapprove.com/ ). Approval/rejection can then be triggered by PR comments and it uses the status API so you know when the PR has passed review. Code review "rules" can get pretty…

That looks great, I'll suggest that we start using it.

Re: More code review tools

#82

Post by zachberger »

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…

Do you know when these changes will come to GitHub enterprise?

Re: More code review tools

#83

Post by tokenizerrr »

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…

I'd also like to voice my interest in force push support. The workflow with Gerrit is very pleasant.

Re: More code review tools

#84

Post by mdo »

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.

I think the goal is to ship a lot more, yes :D.

Re: More code review tools

#85

Post by apinstein »

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…

Reviewable does this really, nicely. We switched over our entire code review process to them (sits on top of GitHub) a few months ago and haven't looked back.

Re: More code review tools

#86

Post by positr0n »

infogulch wrote: post #74
guelo wrote: post #59

Sometimes when I submit a PR and I get a lot of feedback I get lost making sure I've addressed every comment. My #1 feature request would be being able to mark comment threads as resolved. The outdated comment feature sometimes works for this use case but mostly it doesn't.

I didn't see it mentioned but it looks like you can now 'react' to comments, a la facebook. So you could choose one reaction, say, "Horray!", and use it as a marker that you resolved it. Now I wonder if the commenter is notified on every reaction. The only thing that might make this more difficult.

That would also be missing a filter to show all unresolved comments, which is a huge use case. (Or maybe you can do that, if you can it is not very quick and intuitive).

Re: More code review tools

#87

Post by cheshire137 »

infogulch wrote: post #74
guelo wrote: post #59

Sometimes when I submit a PR and I get a lot of feedback I get lost making sure I've addressed every comment. My #1 feature request would be being able to mark comment threads as resolved. The outdated comment feature sometimes works for this use case but mostly it doesn't.

I didn't see it mentioned but it looks like you can now 'react' to comments, a la facebook. So you could choose one reaction, say, "Horray!", and use it as a marker that you resolved it. Now I wonder if the commenter is notified on every reaction. The only thing that might make this more difficult.

> Now I wonder if the commenter is notified on every reaction.

I don't think the commenter is notified about reactions to their comments.

Re: More code review tools

#88

Post by PuercoPop »

js2 wrote: post #45
frutiger wrote: post #44

Earlier quoted context omitted.

Phabricator does this too, but it works even if you force-push to the same branch - it lets you compare the base commit against all subsequent (force-)pushed commits onto the same ref.

Cool. Gerrit requires that each commit message have a "Change-Id: ...." footer so that it can pair up the new incoming commits with the old ones. And you have to push into its magic ref-spec namespace. Neat that Phabricator works by force-pushing over the existing branch, and just keeps track of its previous SHA1s, sorta like a server-side ref-log, exposed via its UI.

A middle ground could be to add the change Id as a git note? But that would probably result in tooling needed to get a good UX flow.

Re: More code review tools

#89

Post by willchen »

I've noticed some open source projects (particularly Angular 2) follow this convention where they never actually "merge" the PR, but rather rebase it into their main branch and have a message in the commit to close the PR.

The advantages seem to be that you get a cleaner git history, and you can keep all the "in-between" / WIP commits that tell the story. Is this done through an automated tooling or is someone manually rebasing it into master? It seems to be a really useful practice so I'm curious how people do it. It would be great if GitHub could offer this natively, as I think many power Git users appreciate the benefits of rebasing over merging.

Post reply on HN