More code review tools • Hacker News Board

Re: More code review tools

#141

Post by alexwebb2 » Wed, Mar 16, 2016, 1:59 PM UTC

majewsky wrote: post #137

eridius wrote: post #107

Earlier quoted context omitted.

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

It is very useful in work cultures where people don't do PRs all the time (particularly out of laziness or to do hotfixes), but you still want to comment on the change.

We do PRs for all changes, but strongly prefer to have comments about a particular commit within a PR be attached to the commit, and not just to the PR.

With this change, if we have comments about a given commit in a PR, we will now have to manually add a link to that commit in the comment so the developer knows which commit the comment is referring to.

It wouldn't be as much of an issue if PR-level code review was easier. As it stands now, the multi-commit view they just added (which is a step in the right direction) is still useless to us because it includes merges commits as well, which is not that developer's code.

So PR-level code review is still a non-starter, and commit-level code review just got clunkier.

I get that they can't support every possible workflow, but they could have at least given us a warning they were going to pull support for a feature we use (they do have those metrics) or at very least acknowledge it somewhere.

Re: More code review tools

#142

Post by oinksoft » Wed, Mar 16, 2016, 4:06 PM UTC

vikiomega9 wrote: post #78

numbsafari wrote: post #4

I'd love it if there was a way for me to queue my comments before submitting. I often keep my comments in a separate textedit/nv window and then go back to put them in since I want to keep track of questions that arise as I'm reading, but I don't want to pepper someone with comments that would be resolved 200 lines later in the diff.

Are 200 line diff common? I feel like having focused patches are useful for everyone. Including git bisect and CI/CD.

The line count of a diff has nothing to do with how focused the patch is. Some PRs are simply larger than others due to the language in use or the nature of the code being changed.

Presumably a codebase in an array-oriented language would have miniscule PRs, which is irrelevant (discussion of PL merits aside).

Re: More code review tools

#143

Post by igor47 » Wed, Mar 16, 2016, 4:57 PM UTC

js2 wrote: post #115

bradfitz wrote: post #95

Earlier quoted context omitted.

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.

bots like these seem to be a natural way to work around github's limitations. realistically, they won't be able to support everyone's use case, but they can provide a platform where lots of different tools can be integrated together.

unfortunately, bots like these are very annoying to write right now. at least one reason is that there is no github event for editing comments. if you try to use webhook events to mirror comments made in github to another code review tool, you will quickly drift out of sync if those comments are edited in github. instead, you have to implement polling, which comes with it's own set of challenges.

also, in general, the event formats are pretty inconsistent. as a random example, in the push event the repo owner only includes name and email, while in the commit comment event the repo owner includes all sorts of information about stars, followers, etc... there are inconsistencies like that all over the place, which means you might need to implement additional API requests for some events and not others.

on a somewhat related note, i've seen a lot of speculation in the open-source community that the reason open-source is getting so little attention from Github is that they're focusing on enterprise customers. i can say that as a somewhat-large enterprise customer, i have yet to get a single issue addressed. examples like above (add a comment-edited and PR-edited event) have been in the ask 6 months or more.

Re: More code review tools

#144

Post by eridius » Wed, Mar 16, 2016, 5:25 PM UTC

alexwebb2 wrote: post #141

majewsky wrote: post #137

Earlier quoted context omitted.

It is very useful in work cultures where people don't do PRs all the time (particularly out of laziness or to do hotfixes), but you still want to comment on the change.

We do PRs for all changes, but strongly prefer to have comments about a particular commit within a PR be attached to the commit, and not just to the PR. With this change, if we have comments about a given commit in a PR, we will now have to manually add a link to that commit in the comment so the developer knows which commit the comment is referring to. It wouldn't be as much of an issue if PR-level code review was e…

As a workaround you could just attach the comment to the very first or last line in the commit diff. Of course, this won't work for commits that consist entirely of non-diffable binary files, but it should work ok in most cases.

Re: More code review tools

#145

Post by piotrkaminski » Wed, Mar 16, 2016, 6:45 PM UTC

numbsafari wrote: post #4

I'd love it if there was a way for me to queue my comments before submitting. I often keep my comments in a separate textedit/nv window and then go back to put them in since I want to keep track of questions that arise as I'm reading, but I don't want to pepper someone with comments that would be resolved 200 lines later in the diff.

Reviewable (https://reviewable.io) is built around this workflow: it will keep your drafts, let you review them, then send them all at once (triggering a single notification email).

Disclosure: I built the tool. :)

Re: More code review tools

#146

Post by piotrkaminski » Wed, Mar 16, 2016, 6:53 PM UTC

danpalmer wrote: post #26

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.

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

Folks who have this problem might want to check out Reviewable (https://reviewable.io):

1. Discussions are automatically treated as "issues" that need to be resolved before the review is complete. Resolving can be as simple as clicking "acknowledge" without following up, but there's also a rich system of "dispositions" that lets you block a discussion, resolve it unconditionally, etc.

2. You can set multiple assignees! (Only one will be reflected in GitHub, though.)

3. You can write a custom rule for determining review completion. One of the samples is "complete only when every assignee has sent an :lgtm:", giving the assignees explicit control over merge approval.

I know you've already decided against Reviewable danpalmer, but I figured others might still be interested. :)

Re: More code review tools

#147

Post by piotrkaminski » Wed, Mar 16, 2016, 6:56 PM UTC

kdazzle wrote: post #140

My biggest pet-peeve with GH code review is that line-comments are automatically folded whenever that line of code is changed. So if the changes to that line didn't relate to your CR or if they didn't actually fix anything, then your comment will pretty much be lost to time. Plus, it would be nice to see the discussion around a particular line without having to go through the entire PR and unfolding each conversation…

Then give Reviewable (https://reviewable.io) a try. :) Comments remain open until acknowledged / resolved and are automatically displayed at the nearest applicable line in every diff.

Disclosure: I built the tool.

Re: More code review tools

#148

Post by piotrkaminski » Wed, Mar 16, 2016, 7:10 PM UTC

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 (https://reviewable.io) has strong support for a rebase-centric workflow while still integrating pretty well with GitHub. Once somebody has reviewed a commit it never disappears, even if you rebase and force push (special refs keep them pinned in the repo). You can easily get incremental diffs between the old and rebased branch or individual commits.

I'll shortly be deploying a new feature where Reviewable will try to match up rebased commits with their predecessors and automatically pair them up when diffing. This is just a heuristic based on commit messages but it works pretty well for non-interactive rebasing. Otherwise, you can still manually diff any two revisions you'd like.

Disclosure: I built the tool.

Re: More code review tools

#149

Post by kannonboy » Wed, Mar 16, 2016, 7:35 PM UTC

krakensden wrote: post #103

slackpad wrote: post #94

Earlier quoted context omitted.

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.

Yep, specifically Bitbucket allows you to create tasks from comments. Bitbucket Server also (optionally) prevents the pull request from being merged until all tasks have been resolved.

Re: More code review tools

#150

Post by wmij » Thu, Mar 17, 2016, 2:07 AM UTC

Myrmornis wrote: post #126

wmij wrote: post #110

Earlier quoted context omitted.

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…

When I do fixup commits, I often think there will be no conflict when it gets rebased but it turns out there is. Do you not worry about mistakes entering at this (post-review) stage?

Yes. It has happened especially as the team has scaled in developer size and when we are in development modes where there's better chances for conflicts, for example iterating on core code during feature development, or when we're touching/adding to common configuration settings. But it hasn't been that often where a developer needed to fix a tricky conflict.

I encourage members on our team to rebase frequently as they're iterating prior to opening their PRs for their individual feature branches and I think this helps us avoid problems for our typical PRs.

Also as a rule, don't sign off on a PR until all conflicts are resolved, if there were any and review the final commit(s) carefully.

We also have the option because of some of the CI tools we've written to give our QA people access to specific SHAs that can be tested, so sometimes (rarely) QA sign-off is asked for as part of the PR.