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

101–110 of 151 posts

Re: More code review tools

#101

Post by alxndr »

erichurkman wrote: post #7
danpalmer wrote: post #2

This is a great step in the right direction. I use GitHub for code review every day, and it has historically been very poorly designed for thorough reviews. These changes look great, I just hope that we get some sort of checking-off of review points, and accept/reject functionality.

I'd love a UI way to enable `?w=1` whitespace mode, especially for html/json/yaml files. I have a feeling a lot of reviewers don't know about it and spend an inordinate amount of time squinting to see what changes when it was a whitespace-only change.

I created a reminder for myself so that every six months I write an email to GitHub asking for someone to create a user option for this whitespace setting.

Re: More code review tools

#102

Post by alxndr »

Negative1 wrote: post #37

Nice additions. Can't help but feel at some point you'll be able to edit and compile code directly on Github (with some sort of compiler/CL backend). Has Github ever discussed integrating Atom directly into the site?

There is a text editor; when viewing a Markdown file for example there's a pencil icon in the top-right of the (rendered) file display.

Re: More code review tools

#103

Post by krakensden »

slackpad wrote: post #94
faitswulff wrote: post #9

Earlier quoted context omitted.

It's interesting that you mention checking off of review points. I had begun to do that in an informal fashion by updating my pull request with notes in the form of a checklist: - [x] Refactor _ - [ ] Rename variable x - [ ] ... Now that you mention it, it would be very nice to have something like Google Docs's ability to mark comments as resolved.

This is always a deal breaker for thorough code reviews - it's always super hard to make sure that everything got resolved in a later commit. The only tool I've seen handle this well is SmartBear's Code Collaborator - you mark certain comments as defects, it does a decent job keeping them aligned with their context as revisions happen afterwards, and then you can close them out before marking the review LGTM. Unfortu…

Bitbucket does this too.

Re: More code review tools

#104

Post by cdcarter »

gmartres wrote: post #93
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…

> 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.

Re: More code review tools

#105

Post by pm90 »

jasonlotito wrote: post #70
grinich wrote: post #24

Earlier quoted context omitted.

They're focused on turning that capital into recurring revenue! i.e. building out the GitHub Enterprise business This is pretty common among companies that make the consumer->enterprise transition. Also see Dropbox, Slack, etc.

> building out the GitHub Enterprise business None of what they just released impacts the GH Enterprise side of things. Maybe it will long down the line, but suffice it to say, none of this sells or keeps customers of GH Enterprise. The reality is, we've questioned our use of GH, and while we are still paying for GHE, it's more or less because it's just not expensive enough at this point to justify switching to somet…

I'm guessing by "focus" on GHE is more on sales and marketing and closing enterprise deals, rather than actual software development. Honestly, I'm really not sure. It might also be perfecting how they deploy to different customers' data centers, how they integrate with their solutions/platforms etc.

Re: More code review tools

#106

Post by viraptor »

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…

Another YES! Gerrit does things right and I'd love GH to support that idea. (lived in gerrit for the last few years)

Re: More code review tools

#107

Post by eridius »

nightpool wrote: post #32
alexwebb2 wrote: post #10

Did they kill commit-level comments? I can no longer make comments on a given commit - the entry box at the bottom is gone. Looks like you have to scroll all the way back up to the top, switch to the "Conversation" tab, and then make a PR-level comment instead of a commit-level comment. I hope I'm missing something here, because as it stands now it's a big step backwards.

They removed full-commit comments on PRs, as opposed to line-level comments on either commits or diffs, or full-commit comments on non-PR commits, but I don't think anyone used those—I haven't even seen anyone use something that wasn't a line comment for a long, long time.

I've used commit-level comments before, but it is admittedly not very common.

Re: More code review tools

#108

Post by eridius »

These are some nice changes, though there's still plenty of things I want GitHub to make better about code reviewing.

One of these changes, and one that annoys me every single day, is when I get an email about a comment, the email doesn't include any context (e.g. it should include the previous comments on that line and probably the hunk as well). And when I click "View on GitHub" to see it in context, if the commit is now on an outdated diff, I get taken to a page that doesn't show the comment at all. It takes me to the Files view, but the Files view doesn't show outdated comments. If the comment is on an outdated diff then it really should take me to the Conversation view with the comment in question expanded.

Re: More code review tools

#109

Post by [deleted] »

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…

[deleted]

Re: More code review tools

#110

Post by wmij »

kderbe wrote: post #30
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…

Have you tried addressing the comments with "fixup!" commits? At the end of review, you can close the pull request, "rebase --autosquash --interactive" the branch, and merge manually.

This is the flow my team uses for addressing any comments, concerns, changes, bugs, etc. We push all subsequent commits to the branch under review and prefix the commit messages with either 'fixup!', 'squash!', etc. that address the comments. The team then reviews the fixup commits further making additional comments or giving a :+1 to the specific commit SHAs. Once everything, including all fixup commits get a :+1 as sign-off will be the only time the developer does a force push to the remote branch under review. Then the developer that owns the PR is responsible for the squash, merge and closing the PR.

Using this flow we never lose any comments during the PR process and can revisit the individual commits, diffs and files changed while the PR is open.

Post reply on HN