4 ms·

I love code reviews and blog posts about them, but I vehemently disagree with all of this advice. >His example PR[0] adds just 152 lines of code, removes 2 lin

I love code reviews and blog posts about them, but I vehemently disagree with all of this advice. >His example PR[0] adds just 152 lines of code, removes 2 lines, but uses 13 thoughtful commits. >While some developers might understand those 152 lines from the final diff alone, I couldn't confidently approve it without the commit story. This is ridiculous! You absolutely can and should review a PR without demanding its "commit story." Go read the PR under discussion here.[0] There's nothing about it that's hard to understand or that demands you go read the 13 intermediate steps the developer took to get there. The unit of change in a code review is the PR itself, not the intermediate commits. Intermediate commits are for the author's benefit, not the reviewer's. If the author rewrote the code in FORTRAN to help them understand the problem, then converted it back to the codebase's language, that's 100% okay and is not something the reviewer needs to care about. The PR should squash the individual PRs at merge time. The linked PR[0] is a perfect example, as the relevant change in the permanent commit history should be "Measure average scheduler utilization" and not "Collect samples" or "Mock sampling." When you need to communicate extra context outside of the code, that should go in the PR description.[1] Your reviewer shouldn't have to go scour dozens of separate commit messages to understand your change. >How do you create a PR that can be reviewed in 5-10 minutes? By reducing the scope. A full feature should often be multiple PRs. A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory. 5-10 minute reviews are so low that it's basically thoughtless rubber-stamping. If someone spent 5-10 hours making a change, the reviewer should definitely think about it for more than 5 minutes. If all the reviewer is doing is spot checking for obvious bugs, it's a waste of a code review. The reviewer should be looking for opportunities to make the code simpler, clearer, or more maintainable. 5-10 minutes is barely enough time to even understand the change. It's not enough time to think deeply about ways to improve it. [0] https://github.com/sasa1977/hamlet/pull/3 https://github.com/sasa1977/hamlet/pull/3 [1] https://refactoringenglish.com/chapters/commit-messages/ https://refactoringenglish.com/chapters/commit-messages/

tantalor 1y ago
Hard agree. Commits are not important. As an author, you should not waste your time on this. As a reviewer, just ignore them.
g-b-r 1y ago
> Intermediate commits are for the author's benefit, not the reviewer's. I don't even know how could commits only benefit the author; if they're poor they won't help him either, if not as a log of how much work he's done. Unless you make a PR for every insignificant change, PRs will most often be composed of series of changes; the individual commits, if crafted carefully, will let you review every step of the work of the author quickly. And if you don't eschew merges, with commits you can also group series of related modifications.
- mtlynch 1y ago
  >I don't even know how could commits only benefit the author; if they're poor they won't help him either, if not as a log of how much work he's done. Intermediate commits are just checkpoints of unfinished code. The author knows that they made them and can revert back to them or use git log --pickaxe-S if there's code they saved to a checkpoint and want to recover. Intermediate commits can have meaningful commit messages if the author chooses, but they could also just be labeled "wip" and still be useful to the author. It's really easy for a note someone writes to themselves to be useful to that person without being useful to other people. If I write a post-it on my desk that says "beef," that can remind me I need to pick up beef from the store, even though if my co-worker reads it, they wouldn't know what the note is trying to communicate. >PRs will most often be composed of series of changes; the individual commits, if crafted carefully, will let you review every step of the work of the author quickly. I don't understand this expectation that an author produce this. What if the author experimented with a lot of approaches that turned out to be dead ends? Is it a good use of the reviewer's time to review all the failed attempts? Or is the author supposed to throw those away and reconstruct an imaginary commit history that looks like their clean, tidy thought process? If someone sends me a short story they wrote and asks for feedback, I can give them feedback without also demanding to see every prior draft and an explanation for every change they made until it reached the version I'm reviewing. The unit of change for a code review is a PR. The intermediate commits don't matter to the reviewer. The unit of change for a short story is the story. The previous drafts don't matter to the reader.
  - g-b-r 1y ago

tantalor 1y ago

Hard agree. Commits are not important. As an author, you should not waste your time on this. As a reviewer, just ignore them.

g-b-r 1y ago

> Intermediate commits are for the author's benefit, not the reviewer's. I don't even know how could commits only benefit the author; if they're poor they won't help him either, if not as a log of how much work he's done. Unless you make a PR for every insignificant change, PRs will most often be composed of series of changes; the individual commits, if crafted carefully, will let you review every step of the work of the author quickly. And if you don't eschew merges, with commits you can also group series of related modifications.

mtlynch 1y ago
>I don't even know how could commits only benefit the author; if they're poor they won't help him either, if not as a log of how much work he's done. Intermediate commits are just checkpoints of unfinished code. The author knows that they made them and can revert back to them or use git log --pickaxe-S if there's code they saved to a checkpoint and want to recover. Intermediate commits can have meaningful commit messages if the author chooses, but they could also just be labeled "wip" and still be useful to the author. It's really easy for a note someone writes to themselves to be useful to that person without being useful to other people. If I write a post-it on my desk that says "beef," that can remind me I need to pick up beef from the store, even though if my co-worker reads it, they wouldn't know what the note is trying to communicate. >PRs will most often be composed of series of changes; the individual commits, if crafted carefully, will let you review every step of the work of the author quickly. I don't understand this expectation that an author produce this. What if the author experimented with a lot of approaches that turned out to be dead ends? Is it a good use of the reviewer's time to review all the failed attempts? Or is the author supposed to throw those away and reconstruct an imaginary commit history that looks like their clean, tidy thought process? If someone sends me a short story they wrote and asks for feedback, I can give them feedback without also demanding to see every prior draft and an explanation for every change they made until it reached the version I'm reviewing. The unit of change for a code review is a PR. The intermediate commits don't matter to the reviewer. The unit of change for a short story is the story. The previous drafts don't matter to the reader.
- g-b-r 1y ago