Hacker News Reader

Comment by ep103 | Hacker News Reader

> 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

View on HN

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.

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.

A diff at meta is a pull-request.

As far as I can see that - and many of the other responses to the criticism - does not counter said criticism.

If you look at only the PR, even with a few lines of context, you still don't see much.

I actually like the diff view window provided by JetBrains editors through the alreedy bundled "Github plugin". I get to see the whole file, before and after (left/right), with highlights for changes, lines added, lins removed. That way I see the entire context.

If you only see the usual change+3 lines of context you often don't even see what function is impacted, and it's also rare to have the context of the entire module being changed in ones head already.

For evaluating PRs, I use the PR review feature in IDEA editors and go through the list of files changed, opening a new window with the entirety of that file available to me, changes highlighted. F7 jumps to the next change, but mostly I just scroll.

You can add review comments right in the diff view.

https://youtu.be/MoXxF3aWW8k ("IntelliJ IDEA. GitHub Pull Requests") -- Diff viewer shown at 2:40; In that video the diff view is shown inside the editor, but I much prefer - and fortunately that is configurable - to open the Diff View in a new window, maximized.

https://www.jetbrains.com/help/idea/comparing-files-and-fold...

The example works only for PRs on GitHub and IDEA editors, but I wanted to present the concept, and show that IMO very nice diff view and how it makes it easy to review changes with the context of the entire file.

Yes I was talking about a pr (i.e. a collection of small diffs usually presented without much context). The smaller the pr the easier to review, but conversely the harder to see the big picture, and most tools don't give nearly enough context around the changes - I prefer to see the entire file.

Thanks for the links.

No. A diff is a commit.

> At Meta we call an individual set of changes made to the codebase a “diff.”

I don't work at Meta and I still don't have any idea which of you is right.

A commit would satisfy the description above.

As would a pull/merge request, which the image in the article perhaps fits better: https://engineering.fb.com/wp-content/uploads/2022/11/Code-R...

Of course, it's possible to review each individual commit as well, but who does that?

I know that some folks love to mess around as much as they need in their local branches, commit often and then do an interactive rebase, to maybe end up with one or just a few commits that can then be the basis for a pull/merge request, then the difference matter a little bit less.

> At Meta we call an individual set of changes made to the codebase a “diff.”

This statement is definitely a misleading one. A diff itself is always a commit in the history log, and a diff can't be a group of multiple smaller units unless their team submit change set to git first and sync the squashed commit back to fbcode. But even in that case, from fbcode's view the squashed commit is still a diff.

A set of changes is called diff stack.

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.

At first glance, I thought you said “comment” instead of “commit”. I laughed at the facetiousness

Meta deploys small changes continuously. If you have a bigger change, it should get broken up into smaller easily reviewable atomic changes.

That seems to be the case, but I think gp’s point still stands.

Imagine a feature that would heavily degrade performances as a whole, but as it’s being introduced in small bits and pieces no one sees the big picture and are left to wonder why perfs are slowly degrading, boiling frog style.

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.

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.

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.

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

> "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.?

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.

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.

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.

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.

Reply on news.ycombinator.com