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

Improving code review time

engineering.fb.com

Post reply on HN
Print view View original

141–150 of 233 posts

Re: Improving code review time

#141

Post by jeffbee »

gknoy wrote: post #98
98codes wrote: post #71

Earlier quoted context omitted.

One thing I've enjoyed where I am now is that PR comments come in two flavors. The first, actual feedback. The second, borderline pedantic issues that are prefaced with "nit: " in the comment. Nit comments are safely ignored but are there so that if the author wants to put in that change while changing some other issue, then OK.

I especially like Github's new option to make a _suggested diff_ of what you want changed. Typo fixes, comments re-worded, etc. It really reduces friction, both as the person making the suggestion, and as the person who authored the PR.

Yes, hopefully someday GitHub will have all the major features that Gerrit has had for years. By the time I'm ready to retire, GitHub PR review UI should be up to approximately 2010 standards.

Re: Improving code review time

#143

Post by CraftThatBlock »

sa46 wrote: post #130
wantsanagent wrote: post #123

Earlier quoted context omitted.

Variation on 2 - We don't care about commit messages at all . All PRs must have good descriptions which use a company-wide template. But individual commits get squashed and their commit messages entirely replaced by the PR template contents. So my commit history is "fump" "GAH" "I think this works?" etc. but all changes have a description of the issue (and link back to the ticket) the solution described in some detai…

Is there any tooling you use to squash the commits and use the PR message?

Github has a "squash and merge" option during merging. You can disable the other options (merge, rebase) in the repo settings.

I've worked with squash and merge almost everywhere and it's great. Keeps history clean (1 commit = 1 ticket, usually), and links back to the PR with discussions and context.

Re: Improving code review time

#144

Post by ep103 »

> Next reviewable diff

Holy Fuck No.

90% of my PR review time goes into "Okay, how will this change impact parts of the system that this mid-level Dev doesn't understand yet?" and "Does this PR actually do what the ticket it is claiming to implement actually intended?"

Reviewing diffs in isolation completely removes one's ability to do that.

If you remove a person's ability to do that, what you've left them with is the easiest part of the PR, just checking that the logic seems logical. And honestly, most of that work can be automated by linting, style cops, and unit tests.

The fact that they got rid of the part of the PR review process that matters, and only saw a 1.5% improvement speaks to all sorts of problems in the process overall, not an improvement by this tool

Re: Improving code review time

#145

Post by netheril96 »

proc0 wrote: post #43

If you're going to add machines to the process why not add it with the purpose of eliminating the human from the process all together? Reviews are necessary because compilers and linters can't catch everything. Runtime bugs that are not caught by the pipeline tend to be edge cases that don't happen until there is enough data to test (in the general sense) the feature. ML could be used for smart testing and if it pass…

Google already has linters and static analysis tools to find out common mistakes or suggest fixes. They complement but not substitute for human reviews though.

Re: Improving code review time

#146

Post by Arainach »

ep103 wrote: post #144

> Next reviewable diff Holy Fuck No. 90% of my PR review time goes into "Okay, how will this change impact parts of the system that this mid-level Dev doesn't understand yet?" and "Does this PR actually do what the ticket it is claiming to implement actually intended?" Reviewing diffs in isolation completely removes one's ability to do that. If you remove a person's ability to do that, what you've left them with is t…

You don't remove their ability to do that. If you need more context, then there are UI elements to show the other lines. At Google, there are also links to the file in question in code search if you want to look at history or any other related context.

Most changes don't need this. A prerequisite of fast code reviews is small changes. Rather than 3000-line features, make a series of changes with 10-100 lines of code plus tests. Reviews can quickly understand the change in logic and confirm that test cases for the new codepaths are being added. Wham, bam, done in two minutes.

Sure, some reviews take more time, but of them 10-30 code reviews I do a week, it's perhaps 10% of them.

Re: Improving code review time

#147

Post by logicchains »

davidmurdoch wrote: post #121

All these comments about how code review is a waste of time, or suggest code review is only for bugs, really shine light on why so much software is incredibly slow today.

Maybe people'd have more time to optimise the performance of their software if they weren't spinning their wheels and context switching waiting hours for minor changes to be merged.

Re: Improving code review time

#148

Post by jackblemming »

bagels wrote: post #125
nomel wrote: post #97

Earlier quoted context omitted.

As someone who has only experienced terrible code reviews, could you give a little summary of what you liked?

Calling you out for lazy work. Teaching you how to use internal tools, company coding standards, suggesting better patterns, digging deep in to the context of system design and maintainability, suggesting other domain experts to tag in the reviews. I learned all sorts of Hack things that obviously I wouldn't learn outside from code reviews.

No post body was provided.

Re: Improving code review time

#149

Post by charcircuit »

godzillabrennus wrote: post #39
andreygrehov wrote: post #25

Meta: > At Meta we call an individual set of changes made to the codebase a “diff.” GitHub: > Pull request Amazon: > Change Request GitLab: > Merge Request Google: > Changelist Nitpicking, but jesus christ, why can't we stick to a single term?

diff request makes the most sense to me... At least that seems descriptive in the context of making a request in a system to get approval to change the code.

A diff request would be asking someone to create a diff for something.

Re: Improving code review time

#150

Post by ok_dad »

Jensson wrote: post #137
ok_dad wrote: post #69

Earlier quoted context omitted.

I enjoy how they state like 3 times that code reviews should be synchronous, yet "industry-standard" (aka: what people really do) is to toss it over the fence in a PR and go back and forth for several days with stylistic bullshit.

Code reviews are usually synchronous at Google though, commenting and fixing things is like chatting with the reviewer so are usually done quickly. Not sure why this wouldn't be industry standard, is there any reason to make code reviews more painful than that?

I was kinda joking that places like Google might have good processes but most places just cargo cult it and do it incorrectly. My current job does reviews that are so useless I don’t even participate anymore and no one cares. I really want to work at a place where they care about code quality and an effective process.
Post reply on HN