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

51–60 of 233 posts

Re: Improving code review time

#51

Post by pavon »

ummonk wrote: post #17
comfypotato wrote: post #5

Am I reading this right? If the total median time in review is “a few hours”, that means that people are dropping what they’re doing to review the code. That can’t really be a good code review? A context switch alone would be half of that time for me.

Working there I tended to conduct code reviews each afternoon at the end of work. Sometimes after coming back from lunch as well. There isn't any "drop what they're doing" involved.

Sure, I do reviews first thing every morning, and sometimes right after lunch (mostly just rereviews), but that would give a mean/median response time of 4 hours assuming work completion time is uniformly distributed. And if changes are requested, that would add another 2-4 hours, which brings the total review-in-wait time to a full day, which the post was saying was unacceptable. To get the numbers they claim they must be reviewing more frequently than that.

Re: Improving code review time

#52

Post by jagged-chisel »

c54 wrote: post #37
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?

I think this basically doesn’t matter and it’s fine for institutional culture to have its own ingroup jargon. Trying to standardize for some aesthetic reason wouldn’t add anything

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

Re: Improving code review time

#54

Post by rubyist5eva »

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?

Mac: What implication?

Dennis: The implication that things might go wrong for her if she doesn't review my code in a timely manner. Now, not that things are gonna go wrong for her, but she’s thinking that they will.

Re: Improving code review time

#55

Post by peteradio »

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…

How would the AI be aware of the business logic communicated via some word doc?

Re: Improving code review time

#56

Post by Quikinterp »

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?

You're thinking of features when the other person is thinking in code. Maybe some overcomplicated bloat is involved with whatever goes into clicking that.

They mean locally as in code scope and not necessarily client sided or the size of a feature

Re: Improving code review time

#57

Post by relueeuler »

anamexis wrote: post #49
relueeuler wrote: post #34

Earlier quoted context omitted.

They are all diffs. Simple is better here.

All pull/change/merge requests are diffs, but not all diffs are pull/change/merge requests.

Right, but in the context of making code changes, you can use diff. Which aligns with the Unix tool diff, and application of said diff with patch. So diff and patch are the operations that happen as you mutate a code base.

Re: Improving code review time

#58

Post by sly010 »

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?

nitpicking but "changelist" sounds like what meta would call a diff-stack (aka multiple diffs).

don't forget Incident = SEV = DEFCON = 911 ...

Re: Improving code review time

#59

Post by Firmwarrior »

jagged-chisel wrote: post #52
c54 wrote: post #37

Earlier quoted context omitted.

I think this basically doesn’t matter and it’s fine for institutional culture to have its own ingroup jargon. Trying to standardize for some aesthetic reason wouldn’t add anything

“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"

Re: Improving code review time

#60

Post by gibolt »

cjtrowbridge wrote: post #46
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?

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.

Post reply on HN