> For a large change, I don’t want to do a “diff review”, I want to do a proper code review of a codebase at a particular instant in time, paying specific attention to the recently changed areas, obviously every team and ticket is different, but IMO unless the person doing the review is some sort of principal engineer mostly responsible for the code at large, this does not align with I would personally consider a cod…
I don't necessarily disagree with you, but I don't agree either.
A typical workflow should be:
1. Engineer writes code 2. Engineer does manual and automated testing to verify things work correctly 3. Engineer commits, pushes, and creates a PR/MR 4. CI runs test suite 5. Another Engineer reviews PR/MR 6. Approval causes merge which causes deployment to staging 7. QA 8. UAT 9. Repeat steps 1-8 until everybody is happy
In this, we need to place checks on #1 and #2.
In my eyes, QA and UAT are superficial reviews/sanity checks on #2.
#3 is a check in #1. However, it relies on A) previous engineers implementing good tests and B) the current engineer doing the same. This means trusting people are doing the right things, but a Review implies you don't completely trust that.
#5 is a check on #3 and #1. They're essential. Having a full understanding of what the code looks like is more important in my mind than what was actually changed. The changes don't show how other code is interacting with those changes (which should be, but isn't always, captured by unit tests).
That's why I think the article author's preferred view is ideal. In fact, that's roughly what I work with when reviewing without realizing it (diff in GitLab/GitHub/Sublime Merge, code in JetBrains where most of the review happens).