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

21–30 of 151 posts

Re: More code review tools

#21

Post by js2 »

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 is somewhat hard to explain unless you're familiar with Gerrit and its "patch set" workflow.

Why? Because my preference is to rewrite history in order to address code review comments, such that what is finally merged leaves a clean history. But it is also valuable during the code review process to be able to look across the revisions to make sure everything has been properly addressed.

The only way to do both, today, is to create a new branch and PR for each "revision". i.e. new-feature, new-feature-rev1, new-feature-rev2. Then close the original PR and reference it from the new PR. A bit tedious.

Re: More code review tools

#22

Post by swanson »

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

You might find this talk valuable -- it's my personal favorite: https://www.youtube.com/watch?v=PJjmw9TRB7s

Re: More code review tools

#23

Post by wclax04 »

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 REALLY wish I could exclude certain file types from the view.

Re: More code review tools

#24

Post by grinich »

smaili wrote: post #19
willchen wrote: post #6

Does anybody else feel like GitHub has released more features in the last month than the last 6 months? I'm not sure if it's just a coincidence with all the attention they've gotten on HN, but these improvements are much appreciated!

Definitely not a coincidence. Although it certainly begs the question of why this wasn't done earlier given the sheer amount of capital they have.

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.

Re: More code review tools

#26

Post by danpalmer »

faitswulff wrote: post #9
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.

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-class support for the concept of people reviewing and accepting (or rejecting) so that it's taken out of the PR description.

Re: More code review tools

#27

Post by tyingq »

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.

This! ?w=1 has existed for years, with no UI way to access it.

Compare two views of the same commit (found this by searching for any commit on github with the word "indent" in it)

https://github.com/douglascrockford/JSON-js/commit/1e3869cb3... (133 additions and 127 deletions)

https://github.com/douglascrockford/JSON-js/commit/1e3869cb3... (30 additions and 24 deletions)

Re: More code review tools

#28

Post by nightpool »

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…

isn't this what the "view outdated diff" button does, or am I misunderstanding?

Re: More code review tools

#29

Post by jordigh »

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…

Mercurial Evolve has a meta-history of which commit rewrites what other commit. It works great, but so far nobody has built a WUI on top of it for code review.

Re: More code review tools

#30

Post by kderbe »

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.
Post reply on HN