Another tip: Use `git log --first-parent` and `git log --merges` to hide the intermediate commits. `--first-parent` also works with `blame` in modern git. These mean you don't have to look at all the small commits when browsing history, only when you want to dive in deeper.
The Theatre of Pull Requests and Code Review
11–20 of 431 posts
Re: The Theatre of Pull Requests and Code Review
#12PR review is probably at least a little performative. But I trust my colleagues to do good reviews when I ask them to, and to ignore my PRs when I don't. That's kind of the way we all want it. I regularly ask for a review of specific changes by tagging them in a comment on the lines in question, with a description of the implications and a direct question that they can answer. This, "throw the code at the wall for in…
> and to ignore my PRs when I don't PRs should be optional, IMHO. Not all changes require peer review, and if we trust our colleagues then we should allow them to merge their branch without wasting time with performative PRs.
There is a difference between a code review and approval to merge/release.
Part of the difference is the idea you can catch all problems with piecemeal code review is nonsense, so you should have at least some sweeping QA somewhere.
Re: The Theatre of Pull Requests and Code Review
#13My guide to good PRs:
- Keep PR messages short and to the point. - use as many commits as you need, it's all the same branch. Squash if you want, I think it hides valuable meta. - put the ticket in the branch name. Non negotiable. - Update your ticket with progres. Put as much details as you can, as if you were writing to someone who's picking up the task after you. - add links to your ticket. Docs, readme, o11y graphs, etc. - Link ticket in PR for easy access to additional info - Admit if you don't understand what your looking at. Better to pair and keep moving forward. - if you request changes, stay available for conversation and approval for the next few hours. - punt the review if you don't feel like you can legitimately engage with the content right now. Make sure you communicate it though. - Avoid nits. This causes a loss in momentum and v rarely is worth changing.
Re: The Theatre of Pull Requests and Code Review
#14A lot depends on your goals for your code reviews. And your goals might even be different for different parts of the code base.
- Are you trying to make sure that more than one human has seen the code? Then simply reading through a PR in 10 minutes and replying with either a LGTM or a polite version of WTF can be fine. This works if you have a team with good taste and a lot of cleanly isolated modules implementing clear APIs. The worst damage is that one module might occasionally be a bit marginal, but that can be an acceptable tradeoff in large projects.
- Does every single change need to be thoroughly discussed? Then you may want up-front design discussions, pairing, illustrated design docs, and extremely close reviews (not just of the diffs, but also re-reviewing the entire module with the changes in context). You may even want the PR author to present their code and walk throuh it with one or more people. This can be appropriate for the key system "core" that shapes everything else in the system.
- Is half your code written by an AI that doesn't understand the big picture, that doesn't really understand large-scale maintainability, and that cuts corners and _knowingly_ violates your written policy and best practices? Then honestly you're probably headed for tech debt hell on the express train unless your team is willing to watch the AI like hawks. Even one clueless person allowing the AI to spew subtlety broken code could create a mess that no number of reviewers could easily undo. In which case, uh, maybe keep everything under 5,000 lines and burn it all down regularly, or something?
Re: The Theatre of Pull Requests and Code Review
#15I mean, stacked PRs are a thing for a reason
I see stacked PRs as independent of this. PRs are a good unit of cohesion of changes, particularly changes that only make sense if later changes are also merged.
Re: The Theatre of Pull Requests and Code Review
#16Can't relate. I take code reviews as possibly the most important part of my job as a developer. Suggesting extra tests, thinking about unintended side effects, and yes, aiming for consistency and readability, without being picky on style choices.
I trust my colleagues to do the same (and they often do).
I can't imagine working in an environment where this is a theater.
Re: The Theatre of Pull Requests and Code Review
#17300 LOC in 10 minutes. Or 2 sec per loc. Or for average 30 char line, 600wpm reading speed. OK. There is little you can review properly in 10 minutes unless you were already pairing on it. You might have time to look for really bad production-breaking red flags maybe. Remember the underlying reasons for PR. Balance between get shit done and operational, quality and tech debt concerns. Depending on what your team need…
Agreed, 300 lines will take me a lot more than 10 minutes to review properly!
Depends on the specific changes of course, but generally speaking.
Re: The Theatre of Pull Requests and Code Review
#18300 LOC in 10 minutes. Or 2 sec per loc. Or for average 30 char line, 600wpm reading speed. OK. There is little you can review properly in 10 minutes unless you were already pairing on it. You might have time to look for really bad production-breaking red flags maybe. Remember the underlying reasons for PR. Balance between get shit done and operational, quality and tech debt concerns. Depending on what your team need…
I like to actually checkout the branch I'm reviewing and run the code myself to observe if it does what is claimed, that usually takes up at least 10 minutes in itself, sometimes more.
From my experience most of the issues I find are actually from this type of observation rather than actually reading the code and imagining what it does in my head.
Re: The Theatre of Pull Requests and Code Review
#19Reviewing someone else's large pull request is like having a second task in parallel with what you are working on yourself!
Re: The Theatre of Pull Requests and Code Review
#20300 LOC in 10 minutes. Or 2 sec per loc. Or for average 30 char line, 600wpm reading speed. OK. There is little you can review properly in 10 minutes unless you were already pairing on it. You might have time to look for really bad production-breaking red flags maybe. Remember the underlying reasons for PR. Balance between get shit done and operational, quality and tech debt concerns. Depending on what your team need…
Your linter/tests are for catching real errors. Review is to understand the shape of it mostly IMO. I could probably fairly easily review 300 loc if its not a particularly confused shape.