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

81–90 of 233 posts

Re: Improving code review time

#81

Post by linuxftw »

bombolo wrote: post #42
KaoruAoiShiho wrote: post #32

Earlier quoted context omitted.

From reading the comment thread I would say the front end would be macro, local would be like a specific function on the front end, like the friends list, that by itself could have "good code".

Like when I have a red number indicating I have a notification and then I click and it just loops forever on some grey animation because it doesn't manage to load the list of notification on my fiber connection?

The little red dot is one microservers, message previews is another, 'load profile picture of message sender id' is probably another, etc.

Re: Improving code review time

#82

Post by erik_seaberg »

gibolt wrote: post #60
cjtrowbridge wrote: post #46

Earlier quoted context omitted.

These are all just terms for slightly different perspectives on the same thing. Like diff is the summary of changes in a commit. the person who wants to affect the change submits a pull request. Another person reviews the pull request and decides whether or not to merge their code into the main codebase, etc. It's all the same thing from different perspectives.

The content is the diff (including description/test plan), which I'd argue should be the name. It could be in a draft state, awaiting review, accepted for merge, or being merged. 'diff' is the shortest at most descriptive name.

When I hear “PR” I think of a tree of git commits. When I hear “diff” I think of one set of squashed text changes, for a tool that doesn’t support git.

Re: Improving code review time

#83

Post by im3w1l »

arcturus17 wrote: post #29
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?

Is diff the best of them all though, because of the implication?

The others have one thing in common: they describe the difference between what is and what should be. Whereas diff is commonly used for the difference between any two things. You could make a diff (as in using the diff program) between two feature branches with no intention of merging one into the other. To me it makes sense to have separate names for these two concepts.

Re: Improving code review time

#84

Post by redwood »

rayxi271828 wrote: post #77
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?

Of all these, the one that confused me the most when I encountered them was surely "pull request".

Same

Re: Improving code review time

#85

Post by proc0 »

twblalock wrote: post #50
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…

Catching bugs is only one of the reasons code review is important. It is also important to transfer knowledge between developers and review design, architecture, scalability, and performance concerns.

I don't know about knowledge transfer. I feel that can be done separately and more effectively. However in the context of this article, if you're going to build an AI tool to aid the process, then why not make an AI that can be trained on a new feature and then tests code changes for bugs. The dev process has been increasingly more and more automated over the years, and I think that won't stop. The current low hanging fruits are things like rolling back the code when something breaks, and testing the code before publishing.. things that companies usually have a lengthy manual process for.

Re: Improving code review time

#86

Post by allenu »

Firmwarrior wrote: post #59
jagged-chisel wrote: post #52

Earlier quoted context omitted.

“Some aesthetic reason” like cutting down on onboarding time? Making things a bit less complex for the juniors? Standardization is more about consistency than aesthetics.

I think most people would be able to handle any one of those terms smoothly on day 1 You: "I'm going to send a pull request" Other engineer: "OK. BTW, we call them change requests here" You: "OK"

If only it were that simple. As we’ve seen in this thread, you’d instead likely have:

Somebody else: Where I used to work we called them pull requests. Let’s have a meeting to discuss changing the terms to match industry standards. We may also want to create a committee to agree on other terminology changes too, like main instead of master.

Re: Improving code review time

#87

Post by imiric »

jiveturkey wrote: post #48
bagels wrote: post #38

Here's another factor at Meta that can reduce code review time: Your performance review is based in part (maybe not a large part, but in part) on how many reviews you perform, and how many words you put in to those reviews. edit: In short, people are incentivized to review

I quite agree with the proposition you are making here. It goes to the very heart of the matter, does it not. The better you are at giving detailed, explicit and concrete feedback about each and every particular aspect of a diff (NOTE: both good and bad), the better and more competent you clearly are, and the more of a true champion for the cause you are proving to be. Time and time again, I wish my reviewers would j…

Meh.

I've often seen reviewers delivering essays to back up their arguments, which essentially boil down to "because I prefer it this way".

Focusing on pure word count doesn't mean that the feedback is valid, or even explains the reasoning well. If anything, it encourages nitpicky and long winded comments based on personal preference.

Often, less is more. If you can get a point across by a small code suggestion, do that instead. But definitely don't fall into the trap of suggesting huge chunks of code, or rewriting parts of it. Sometimes even asking a question to improve understanding is better than arguing a point.

And then other times, especially for trivial changes, "LGTM" or just a blank approval is perfectly fine as well. No need to waste time discussing trivial things if everyone is on the same boat.

Re: Improving code review time

#88

Post by matai_kolila »

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?

Other folks have said the basic (these aren't the same) but one thing that I feel like few people realize is that "Pull Requests" aren't actually a git concept at all.

"Pull Requests" are just implementations that many web-based git tools (but not all!) have adopted to facilitate the code review process taking place on their platforms.

Re: Improving code review time

#89

Post by allenu »

posharma wrote: post #3

Disclaimer: these are anecdotal reports. I've heard from a lot of my friends how abysmal the quality of code is at Meta. Obviously, this may not be true in all teams/products, but that's the general sentiment. Why make it faster when you're already dealing with mess! This is abundantly evident from the constant fire fighting, duct tapes and a metric driven culture that incentivizes the number of diffs landed.

If what you’re saying is true, I wonder how much of the quality issues is just a company getting really big over time. Harder to standardize or keep tabs on such a large number of developers.

Re: Improving code review time

#90

Post by [deleted] »

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?

[deleted]
Post reply on HN