Code Review Guidelines

Purpose

Code review exists to catch bugs, share knowledge, and maintain code quality. It does not exist to relitigate architectural decisions that were made six months ago.

Process

1. Open a pull request
2. Assign at least one reviewer
3. Reviewer reviews within 24 hours (48 hours if it's a large PR, 1 week if it's a PR from the intern)
4. Author addresses feedback
5. Reviewer approves
6. Author merges

What to Look For

• Does the code work?
• Is it readable?
• Are there tests? (Aspirational at TomlTech, but we're trying)
• Are there obvious security issues?
• Does it follow the existing patterns in the codebase, even if those patterns are questionable?

What Not to Do

• Do not rewrite the entire PR in your review comments
• Do not leave "nit" comments on code that is functionally correct and readable
• Do not request changes because you would have done it differently
• Do not approve without reading the code (we know you do this, please stop)
• Do not leave the comment "looks good to me" on a PR that deletes the authentication system

Style

We use Rubocop with the default configuration plus 14 custom rules that nobody remembers adding. If Rubocop passes, don't argue about style. If you disagree with Rubocop, open a PR to change the Rubocop config. Nobody has ever done this because it requires understanding the Rubocop config, and the Rubocop config is 400 lines long.

Turnaround Time

Reviews should be completed within 24 hours. If a review is not completed within 48 hours, the author may merge without review. This rule was added after a PR sat open for 3 weeks and the branch diverged so far from main that the merge conflict was larger than the original PR.

The "Gary Rule"

Named after a former employee. If a PR introduces a gem dependency, the reviewer must verify that:

1. The gem is actively maintained
2. The gem has more than 3 downloads
3. The gem author is not named Gary