pr_checklist.md

pr_checklist.md

Elixir pull request checklist

0. Structural

• 0.1. No duplicated code
• 0.2. Single Responsability Principle
• 0.3. Open-Closed Principle
• 0.4. Low coupling

1. Low-hanging-fruits

• 1.1. # Comments
• 1.2. Typos
• 1.3. Leftover @tag, @describetag, and @moduletag
• 1.4. Decreased coverage (mix coveralls)
• 1.5. Incorrect filenames
• 1.6. Are optimizations being benchmarked?

2. General

• 2.1. Are all tests calling the thing their describe tells it is?
• 2.2. Function definitions
  • 2.2.1. Are there public functions that could be private?
  • 2.2.2. Are there public functions that are not being used?
  • 2.2.3. Heuristic: when a function call is removed, check if it used somewhere else - if not, its definition can also be removed.
• 2.3. Could any apply_changes be replaced by get_fields?
• 2.4. Are there complex conditionals that could be replaced by pattern matching?
• 2.5. Are there magic numbers?
• 2.6. Are async tests being used when possible?
• 2.7. Are there n + 1 queries (logical ones)?

3. Potential bugs

• 3.1. Are required fields from schema not being required in the migration?
• 3.2. Are required fields from migration not being required in the schema?
• 3.3. Check for casting mistakes
  • 3.3.1. Are types correctly defined for the schema?
  • 3.3.2. Are required fields not being casted?
• 3.4. Are all field defined in a factory really castable??
• 3.5. Is the unhappy path for a route being tested?
• 3.6. Are > and < operators being used as Date.compare/2?

4. Migrations

• 4.1. Check for long migrations: are there any migration with too many statements?
• 4.2. Are there removed columns that still may being used by some application instances? (Solution: phased deployment)
• 4.3. Are there table locks occasioned by default values insertions? (E.g. add :is_abroad, :boolean, default: value, null: false)
• 4.4. Backfilling data migrations have transactions disabled and are using batch updates? Do they have a throttling (to avoid CPU spike)?
• 4.5. Are there renamed tables that still may being used by some application instances?
• 4.6. Are indexes being added without the concurrently: true option?
• 4.7. Is the migration name correct?
• 4.8. Uses :utc_datetime_usec for timestamps?
• 4.9. If constraints are added, do existent rows conflict with the new constraints?

5. Style

• 5.1. Are validations next to the changeset function?
• 5.2. Is there any function that could be inline but isn't?
• 5.3. Is pattern matching being used as a type checking mechanism?
• 5.4. Map.put/3 usage: use %{map | key: value} instead.
• 5.5. join_through uses module instead of string?
• 5.6. Are feature flags named after the feature? E.g. enable_mailbox instead of enable_gmail_smtp_adapter