> unless the person doing the review is some sort of principal engineer mostly responsible for the code at large [..] In my book a general code review is simple sanity check by a second pair of eyes
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).
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).