Notes from Google’s Engineering Practices documentation
- CL: Stands for “changelist,” which means one self-contained change that has been submitted to version control or which is undergoing code review. Other organizations often call this a “change” or a “patch.”
- LGTM: Means “Looks Good to Me.” It is what a code reviewer says when approving a CL.
- emergency: small change allows a major launch to continue instead of rolling back, fixes a bug significantly affecting users in production, handles a pressing legal issue, closes a major security hole
- Purpose - make sure the overall code health of Google’s code base is improving over time
- In General - reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on
- There is no such thing as perfect code only better code
- Rather than profection, a reviewer should seek continuous improvement
- Prefix non-critical comments with “Nit: ”
- Never worsen a codebase unless it’s an emergency
Sharing knowledge is part of improving the code health of a system over time.
- Make sure it’s clear that this is for education and not critical for merging the changelist
- Technical fact over preferences
- Styleguide or it’s personal preference
- If an author can demonstrate that there are several valid approaches, CR should defer to author
- author may ask for consistency if that doesn’t worsen overall codehealth
- resolve conflict using guidelines
- Have a meeting, but record results of the meeting on the patchset
- escalate to team lead or eng manager, don’t let a CL sit around due to conflict
- Does this “make sense”?
- Is this where this code belongs?
- Is now the time to make this change?
- Does this integrate well with the rest of the system
Mostly, we expect developers to test CLs well-enough that they work correctly by the time they get to code review.
…disagree
- Does what the dev intended
- We want what the dev intended
- UI changes in particular
- parallel programming: think really hard about it
- regexes, too, I reckon
- too complex can’t be understood quickly by code readers/developers likely to introduce bugs when modifying this
- over-engineering - solving future problems that we don’t know about yet
- Add tests with CL that adds functionality
- Tests must “be sensible”
- Break when code breaks, work only when code works
- Don’t accept complexity
- “good names”
- Long enough to describe what it does without being too long to read
- Understandable
- Explain why code exists not what code is doing
- Maybe this change resolves or conflicts with another comment
Don’t block CLs from being submitted based only on personal style preferences.
- Don’t make style changes with other changes
- Check to see if changes how code is build, tested, released, interacted with
- Deprecated code? remove/indicate in documentation
- Don’t skip over any part of the code review
- Make sure you understand what the code does
- Make sure you tell the reviewer if you don’t understand the code
- File context
- System context
- Say something nice
- More helpful in mentoring terms
- Check the description: what + why
- Does the change make sense?
- Reject with suggestion of what dev should have done if possible
- Lots of changes you don’t want?
- post your dev process more publicly
- If you can’t find the “main part” ask, or ask to split this up into multiple CLs
- See if you see “Design problems” in the “main part”
- Send that first since:
- Devs could submit work on top of a CL
- People have deadlines
- Read all the files
- Maybe read tests first
- team velocity goes down (cycle time)
- developers protest the process entirely
- code health could go down as pressure goes up
- one business day
- don’t interrupt yourself though
- time to first response vs time to lgtm: only concerned with the former
- especially across timezones
- split it into smaller CLs
- quick comments on design first
- unblock the developer
- emergencies and hard deadlines
- Be nice
it is the developer’s responsibility to fix a CL, not the reviewer’s
- Balancing “fix a CL” w/unblocking
- Asking a developer to clarify == comment or simplify
- sometimes it’s them, tell them and drop it
- Be polite, demonstrate you hear what they’re saying, but if it’s about code health, insist on it
- Doesn’t happen a lot
- More about tone than content
- Doesn’t happen
- Common way for codebases to degenerate
- New complexity? Clean it up
- Exposes surrounding problems? File a task, add a TODO to the CL
- This is a complaint about speed
- May take a while for them to see any value in strictness
- LGTM != merge && LGTM != deployment
- I wonder if +2 == merge holds up CR?
- In general favor merging patchsets
- “Code Health” needs a definition here
- I like the mentoring/pedegogy part
- Technical Facts are hard to come by, in my view
- reads like a checklist
- “Mostly, we expect developers to test CLs well-enough that they work correctly by the time they get to code review.” is a thing I don’t think is true
- Good Things is undervalued, I think
- Good points about comments and documentation
- Another thing I’d add is spelling - weirdly this comes up a lot
- I think more guidance is needed for “design” – what does “make sense” mean for a CL?
- under-emphasis on change description
- It’s in the developer part of this documentation
- Less than 24 hrs to first response, not LGTM
- “Be nice” heuristic: if you’re writing “obviously” or “clearly”, you’re being an asshole
- There is a power imbalance here
- High context vs low context
- Cleanup never happens
- Most complaints are about CR speed