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