5 ms·
I had the pleasure of being at a startup with a very talented SRE named M. Harrison. His minimum-level review criteria were that when you were planning to appro
by clipradiowallet 5y ago
I had the pleasure of being at a startup with a very talented SRE named M. Harrison. His minimum-level review criteria were that when you were planning to approve a PR, you should summarize everything the PR does in the approval comment. Responses like ":+1:" or "LGTM" were not allowed. When I would read a PR intending to understand and then summarize, the errors or omissions (eventually) started to just jump out at me. I'm not sure if I'm articulating it properly...but that expectation of a small book report-ish paragraph helped me become much better at critically reviewing PRs.
disclaimer: the majority(not all) PR's were in a very very large terraform codebase.
- cbsmith 5y agoInteresting. If before you weren't intending to understand and be able to summarize before, what were you doing when you reviewed code?
- deleted 5y ago[deleted]
- mandelbrotwurst 5y agoWriting the summary is a higher bar than just deciding that you could.
- reidjs 5y agoWouldn’t most people just restate the ticket? Or do you mean, they write about the actually implementation. Eg, “this fixes the race condition by removing the timeout and waiting until the initialization task finishes (line #55) before instantiating the chart widget”
- edhebert 5y agoThe latter would be a very useful code commit message, (the line #55 may be excessive), both for the author and the reviewer.
- duped 5y agoI long for tickets that descriptive
- reidjs 5y agoThe ticket would probably just say “this thing doesn’t load correctly sometimes”, my description was for the code review
- cbsmith 5y agoI agree. I just was curious if that was the only difference. The way it was phrased it sounded like it wasn't.
- clipradiowallet 5y agoHere's a contrived example... Let's say a PR arrived that appeared to create a new S3 bucket and a new IAM role with an attached policy that grants read-only permissions to that bucket. The old me would read it, and write something like "adds S3 bucket and IAM role". The newer me would write it as: 1. Creates S3 bucket <XYZ> in account <DEF>. 2. Creates IAM role <ABC>, with an attached policy granting read-only access to S3 bucket <XYZ>. It seems like a small change, but when I'm actually planning to type out those details - I'm more likely to notice typos in the name/account/role/etc, or just outright errors. One of the mental shifts from my old reviews to newer ones is that I open the PR assuming I don't know what it does yet. So I have to read it for myself and interpret it, then summarize it for someone else.
- cbsmith 5y agoThanks, this helped clarify for me what your experience was like. It's interesting.
- LeftHandPath 5y agoAt work, I've been writing a word document to explain every part of a program I've written and how each element interacts with the rest. It's been very helpful to find things that don't make as much sense as I thought they did when I wrote them, and revealing old code that is no longer used anywhere in the application. And I've caught a few places where outdated .txt / .md / inline-doxygen documentation lies, too. Writing a summary document really forces you to get to the nitty gritty and catch things that a casual review would miss.
- chii 5y agoBut wouldn't this summary document be better as a form of comment in the source code rather than a separate document that will become out of date the next time someone else makes changes?
- KronisLV 5y agoFurthermore, wouldn't a Markdown document be a bit better as opposed to Word? That way, you or anyone else could easily search through its contents alongside the rest of the codebase, were it to be included in the repository, whereas that wouldn't work with Word, because it's a proprietary and binary format (unless you open that file in particular).