active OSS projects EVAR! • Hundreds of repos on github • Contributors from every timezone and almost every continent (Antarcticans - if you’re out there, holla!) We receive scores to hundreds of Pull Requests per day Every single PR needs someone to review it! Why do I care?
a single sitting is best Get into the mindset • Close email, chat, Slack, IRC, Zoom, etc. • Headphones or similar environmental neutralizers Read the associated bugs • There ARE bugs linked, right? Understand WHY the change is being made Before starting
fixing? Does this actually fix it? Does this prove that it fixes it (tests, benchmarks)? Is this the “right” fix? If not - it’s OK to push back Motivation: Bug fix
Can it be broken down further? Are functions, types, variables named clearly? • Go style encourages short names for locals, but sometimes “frobber” really is better than “f” • Code should “read well” Whitespace can be as valuable as code to clarity Clarity
focused PRs are better Is it complete? • Opposite of above - don’t commit half-cooked work Is it exposing details that callers don’t need to know? • Can it be pre-factored to reduce scope? Scope
E.g. breaking out a function for readability vs exporting The more widely visible, the more the API and generality matters • Hyrum’s law - all exposed surface will be used • Special cases for things like k/utils Generality
• Almost never “what” or “how”, unless obscure • If you did some analysis to make a decision, say what • If you chose something at random, say that • If you think you will need to revisit some decision later and you are leaving room for that, say so If the next reader’s instinct will be to undo some of this, make sure to leave a note Comments
DO NOT IGNORE (or comment why) • Wrap errors when context is needed Think about corner-cases and “pathological” situations Does it fail gracefully? • Will it retry? • Is this clear in the code and comments? • How will the user know something failed? Errors and failure
logs useful and/or actionable? • Are they spammy? • Do they have enough data to comprehend what actually happened? Does it have metrics, when appropriate? • Do they tell us something we actually want to know? Observability
• Think about how they will be used: do they feel natural? • Are they consistent with other instances? • Would they impede or confuse other planned work? • Principle Of Least Surprise • If we were starting it all over, what we would want? UX
PR without tests is an immediate smell • Even if tests didn’t exist before, do better Warning signs • Do the tests require twiddling some global variable or flag? • Do the tests anticipate the most obvious ways for future me to break your feature? • Are the tests at the smallest scope possible? • Do the tests use time.Sleep? Testing
etc. These sorts of changes must be well commented These PRs really should come with a benchmark • To demonstrate efficacy • To prevent regressions Performance and scale
Client and server • All the functionality for a whole change We don’t want to merge half-complete features • It’s too easy to miss a release cutoff • OK to seek review before complete, but not merge Completeness
been done in the past • Why things were done the way they were • What performance metrics matter or have been a problem in the past • How other features will interact with this change • What we want to do in the future Domain specific knowledge
IT! • Go exploring - software archaeology is fun • git grep is your friend • Do experiments or patch in the change and try it • Ask for a consult • Ask PR author to document the results in comments • links to previous issues / changes / discussions are VERY helpful for future others I’ve seen too many PRs approved without sufficient context that need to be fixed later • I have been guilty of this, too Domain specific knowledge
happened and why Organize large PRs into intentional, discrete steps • Easier up-front, but possible ex post facto Commits should almost always stand on their own OK to have “fix feedback” commits during review • Need to fold those back to real commits BEFORE merging • Yes it is tedious, sorry (not sorry) Commit hygiene
should summarize, explain what the PR is about and why, link to relevant docs (e.g. KEPs) and reference bugs Commit comments should be thorough and detailed • Enough for git log to be useful • WHY > HOW PR and commit comments
to better accommodate a future change • Reduces the size and scope of the real change • Should be no-op code changes, plus tests Prefactoring is not just acceptable - encourage it! • Any significant change will almost certainly require it • Can be separate PRs or commits in a larger PR Prefactoring
following this correctly?” • “Why was this choice made?” • If you send every comment you write, you are probably doing it wrong DO: Explain your thinking • “If X is true, then Y can’t be true, but this doesn’t handle that case” Communication
Nit: A small thing that could be improved • Required: “I will not /lgtm until you fix this” DON’T: Pick a bunch of nits if the real problem is that the PR has the wrong approach • Architectural guidance should be first priority Requesting changes
reviewing or not, or what state you think the PR is in overall • It’s frustrating to get batch after batch of comments when you thought you were done DON’T: Approve because of deadlines or guilt • There’s always another release • Again: There’s. Always. Another. Release. Progress
the defenders of this codebase • THIS IS YOUR JOB (for the moment) DO: Be reasonable • They won’t do everything exactly how you would have DO: Be willing to be wrong; trust, but verify DO: Be willing to acquiesce on reasonable debate Discussion
debt and tradeoffs with your SIGs and other reviewers, if needed This project is massively distributed - we HAVE TO trust each other to make good decisions Tradeoffs
lightly • Make time, and don't half-ass it • We have to live in the codebase we create • Trust your instincts: if it doesn’t smell right, investigate • You’re guaranteed to find either a bug in the code or a bug in your instincts • Do not approve things you don’t understand fully At the end of the day...