4 ms·

> 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

by ep103 4y ago

> 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

Arainach 4y ago
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.
solatic 4y ago
> "Does this PR actually do what the ticket it is claiming to implement actually intended?" Let me ask about your unspoken assumption: is this the PR reviewer's job? Maybe the PR seems to implement what the ticket asked for, then after merging it becomes clear that it didn't fully implement it, or the business stakeholders are unsatisfied, etc.?
- ohgodplsno 4y ago
  It's not the PR reviewer's job to go actively test it out (you can assume that your colleagues are somewhat competent at what they're doing), but if you review with the spec or the issue open on the side that says to add a blue button and you see it's red, it's your job to ensure it's not a mistake and point it out to the author.
lopkeny12ko 4y ago
I don't understand this criticism. How does a "next reviewable diff" pop-up suggest that you're being forced to review a diff "in isolation"? As I understand it, nothing proposed in this article prevents you from reviewing the diff in context of the larger piece of software, as you would have always done. This just seems like a feature to suggest another diff for you to review after you've finished accepting/rejecting the current diff. I'm already "in the zone" of code review so to speak, so this minimizes context switching. I see this is a good thing. > The fact that they got rid of the part of the PR review process that matters I don't understand this either. What "part of the PR review process" did they remove? The article does not claim to have eliminated any part of the review process.
- grey-area 4y ago
  It encourages looking quickly at small bits of changes in isolation as opposed taking a holistic view of the entire change, and viewing them in series as 5 min little tasks you can tick off like tiktok videos. The review tools on github already go too far toward this by removing far too much context.
  - pistoriusp 4y ago
    A diff at meta is a pull-request.
    - yuhc 4y ago
      No. A diff is a commit.
      - zeckalpha 4y ago
        They might get squashed for review or before merge, making the distinction less important. When you are running a monorepo you generally don't get long series of small commits in a batch but rather a singular atomic commit.
      - KronisLV 4y ago
charcircuit 4y ago
>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 The 1.5% improvement was from better suggestions on who would be a good person to review the diff. The next reviewable diff feature "resulted in a 17 percent overall increase in review actions per day (such as accepting a diff, commenting, etc.) and that engineers that use this flow perform 44 percent more review actions than the average reviewer!"
kqr 4y ago
You seem to be thinking of "hunk" and not what Facebook calls "diff" -- what Facebook means by "diff" is closer to what many people call "commit" or even "branch", i.e. a set of interrelated changes that are sort of atomic. Reviewing individual hunks would be crazy and even Facebook knows that.
thehm 4y ago
Those architectural decisions should be reviewed during the design and planning phase so mid/low-level devs don't waste time building the wrong thing in the first place.
- ckdot 4y ago
  In theory that’s right what you say. But still, tasks should be small enough that it hopefully won’t hurt too much if you throw away all the code again. Too often I experience that - even if you talk to low/mid level devs about a feature before, even if you make a task breakdown together with them and write all the software design decisions down, even if you tell them they should commit often to you can check once in a while, even then in the end it‘s too often garbage what has been produced. Still, companies want to keep these developers because it’s hard to find new ones. And I guess it’s our senior’s duty - even if there are many disappointments - to still assume the best and try to teach them to do better. Again and again and again.
craig 4y ago
I think you are misinterpreting the term "diff". Near the beginning of the article they describe it "At Meta we call an individual set of changes made to the codebase a “diff.” So diff == PR.
SirensOfTitan 4y ago
Diff at Meta is short for "Differential revision," a Phabricator term (which was originally an internal FB project before Evan left). The fact that this comment still remains at the top here despite being incredibly inaccurate (and many subcomments stating as such) shows how degraded the discussion has gotten here when Meta is mentioned.

by ep103 4y ago

Arainach 4y ago

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.

solatic 4y ago

> "Does this PR actually do what the ticket it is claiming to implement actually intended?" Let me ask about your unspoken assumption: is this the PR reviewer's job? Maybe the PR seems to implement what the ticket asked for, then after merging it becomes clear that it didn't fully implement it, or the business stakeholders are unsatisfied, etc.?

ohgodplsno 4y ago
It's not the PR reviewer's job to go actively test it out (you can assume that your colleagues are somewhat competent at what they're doing), but if you review with the spec or the issue open on the side that says to add a blue button and you see it's red, it's your job to ensure it's not a mistake and point it out to the author.

lopkeny12ko 4y ago

I don't understand this criticism. How does a "next reviewable diff" pop-up suggest that you're being forced to review a diff "in isolation"? As I understand it, nothing proposed in this article prevents you from reviewing the diff in context of the larger piece of software, as you would have always done. This just seems like a feature to suggest another diff for you to review after you've finished accepting/rejecting the current diff. I'm already "in the zone" of code review so to speak, so this minimizes context switching. I see this is a good thing. > The fact that they got rid of the part of the PR review process that matters I don't understand this either. What "part of the PR review process" did they remove? The article does not claim to have eliminated any part of the review process.

grey-area 4y ago
It encourages looking quickly at small bits of changes in isolation as opposed taking a holistic view of the entire change, and viewing them in series as 5 min little tasks you can tick off like tiktok videos. The review tools on github already go too far toward this by removing far too much context.
- pistoriusp 4y ago
  A diff at meta is a pull-request.
  - yuhc 4y ago
    No. A diff is a commit.
    - zeckalpha 4y ago
      They might get squashed for review or before merge, making the distinction less important. When you are running a monorepo you generally don't get long series of small commits in a batch but rather a singular atomic commit.
    - KronisLV 4y ago

charcircuit 4y ago

>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 The 1.5% improvement was from better suggestions on who would be a good person to review the diff. The next reviewable diff feature "resulted in a 17 percent overall increase in review actions per day (such as accepting a diff, commenting, etc.) and that engineers that use this flow perform 44 percent more review actions than the average reviewer!"

kqr 4y ago

You seem to be thinking of "hunk" and not what Facebook calls "diff" -- what Facebook means by "diff" is closer to what many people call "commit" or even "branch", i.e. a set of interrelated changes that are sort of atomic. Reviewing individual hunks would be crazy and even Facebook knows that.

thehm 4y ago

Those architectural decisions should be reviewed during the design and planning phase so mid/low-level devs don't waste time building the wrong thing in the first place.

ckdot 4y ago
In theory that’s right what you say. But still, tasks should be small enough that it hopefully won’t hurt too much if you throw away all the code again. Too often I experience that - even if you talk to low/mid level devs about a feature before, even if you make a task breakdown together with them and write all the software design decisions down, even if you tell them they should commit often to you can check once in a while, even then in the end it‘s too often garbage what has been produced. Still, companies want to keep these developers because it’s hard to find new ones. And I guess it’s our senior’s duty - even if there are many disappointments - to still assume the best and try to teach them to do better. Again and again and again.

craig 4y ago

I think you are misinterpreting the term "diff". Near the beginning of the article they describe it "At Meta we call an individual set of changes made to the codebase a “diff.” So diff == PR.

SirensOfTitan 4y ago

Diff at Meta is short for "Differential revision," a Phabricator term (which was originally an internal FB project before Evan left). The fact that this comment still remains at the top here despite being incredibly inaccurate (and many subcomments stating as such) shows how degraded the discussion has gotten here when Meta is mentioned.