Last Updated: 2024-11-23

Each section is ranked by importance, with the most important first

1. API semantics

• Is there one way of doing things instead of many?
• Is it consistent/least suprises
• Clean split of internals/API surface area (i.e. min leaking)
• Is API generally useful and not overly specific
• Any breaking changes to user-facing parts (which can mean other callers of the API)

2. Implementation

• Are the original requirements satisfied?
• Performant?
• Secure (i.e. no SQL injection)?
• Observable (logs, metrics, tracking, etc.)
• Do newly added dependencies pull their weight? Is there license acceptable?

3. Documentation

Does the UX explain the feature to the user? Are API docs updated? Are docs understandable (typos/grammar)? Is the README updated? Are there code comments as needed?

Bad documentation repeats what the code says. Good documentation supplies the contains the context that is missing from the code.

4. Testing

Are there appropriate tests added?

5. Formatting

Does it follow code guidelines? Watch out for later commits added after initial review.

Reinstall dependencies if there is an issue

When doing a code review for M, his tests failed on my machine and I wrongly told him the code was failing tests. The issue was that I did not install the latest packages. This was silly of me. I should always install them first if tests fail.

The same can apply for similar commands - e.g. to refresh cache data etc.

Make sure language updates match code changes

If a PR changes something like ratings being in the last year to ratings being all time, then check that the language in the markup is reflected to update it.