|
| 1 | +## Code Review Guidelines |
| 2 | + |
| 3 | +When reviewing code in this repository, apply these standards strictly. |
| 4 | +### Language Hierarchy |
| 5 | + |
| 6 | +**Preferred:** |
| 7 | +1. Zig — wherever C or C++ would be considered; low-level, comptime, no hidden control flow |
| 8 | +2. Rust — systems, CLI, performance-critical |
| 9 | +3. Ada/SPARK — safety-critical, formal verification |
| 10 | +4. Haskell — pure functional, type-heavy domains |
| 11 | +5. Elixir — concurrent, distributed, fault-tolerant systems |
| 12 | +6. ReScript — frontend when JS interop needed |
| 13 | +7. Chapel — parallel computing, HPC workloads |
| 14 | +8. Julia — numerical computing, scientific applications |
| 15 | + |
| 16 | +**Avoid:** |
| 17 | +- C — use Zig instead |
| 18 | +- C++ — use Zig or Rust instead |
| 19 | +- Python — reject unless interfacing with Python-only libraries |
| 20 | +- JavaScript — use ReScript or TypeScript instead |
| 21 | +- Shell scripts over 50 lines — rewrite in a proper language |
| 22 | + |
| 23 | +**Flag for justification:** |
| 24 | +- Any use of Go, Java, C# without clear rationale |
| 25 | +- C or C++ where Zig would suffice |
| 26 | +--- |
| 27 | + |
| 28 | +### Error Handling |
| 29 | + |
| 30 | +**Rust:** |
| 31 | +- No `.unwrap()` or `.expect()` without a comment justifying why panic is acceptable |
| 32 | +- Prefer `?` operator for propagation |
| 33 | +- Use `thiserror` for library errors, `anyhow` for application errors |
| 34 | +- No `panic!` in library code |
| 35 | + |
| 36 | +**Haskell:** |
| 37 | +- No `error` or `undefined` in production code |
| 38 | +- Use `Either`, `Maybe`, or `ExceptT` for fallible operations |
| 39 | +- Partial functions must be justified |
| 40 | + |
| 41 | +**Elixir:** |
| 42 | +- Use `{:ok, _}` / `{:error, _}` tuples consistently |
| 43 | +- No bare `raise` without rescue strategy |
| 44 | +- Supervisors must have explicit restart strategies |
| 45 | + |
| 46 | +**Ada/SPARK:** |
| 47 | +- All exceptions must be documented |
| 48 | +- Prefer preconditions/postconditions over runtime checks |
| 49 | +- SPARK contracts required for safety-critical sections |
| 50 | + |
| 51 | +--- |
| 52 | + |
| 53 | +### Documentation |
| 54 | + |
| 55 | +**Required:** |
| 56 | +- All public functions/types must have doc comments |
| 57 | +- Module-level documentation explaining purpose |
| 58 | +- Examples for non-obvious APIs |
| 59 | +- README must explain: what, why, how to build, how to use |
| 60 | + |
| 61 | +**Format:** |
| 62 | +- Use AsciiDoc (`.adoc`) for documentation files, not Markdown |
| 63 | +- Exception: GitHub-required files (e.g., this file) |
| 64 | + |
| 65 | +**Flag if missing:** |
| 66 | +- CHANGELOG entries for user-facing changes |
| 67 | +- Architecture decision records for significant design choices |
| 68 | + |
| 69 | +--- |
| 70 | + |
| 71 | +### Tooling Preferences |
| 72 | + |
| 73 | +**Flag violations:** |
| 74 | + |
| 75 | +| Violation | Should Be | |
| 76 | +|-----------|-----------| |
| 77 | +| Docker | Podman | |
| 78 | +| Makefile | justfile | |
| 79 | +| GitHub Actions self-reference | GitLab CI preferred for personal projects | |
| 80 | +| npm/yarn | pnpm (if JS unavoidable) | |
| 81 | +| pip/poetry | Reject (avoid Python) | |
| 82 | + |
| 83 | +**Build files:** |
| 84 | +- `justfile` must be present for any project with build steps |
| 85 | +- Recipes must have descriptions (`# comment above recipe`) |
| 86 | + |
| 87 | +--- |
| 88 | + |
| 89 | +### Code Style |
| 90 | + |
| 91 | +**General:** |
| 92 | +- No commented-out code — delete it (git has history) |
| 93 | +- No TODO without linked issue |
| 94 | +- No magic numbers — use named constants |
| 95 | +- Functions over 40 lines should be justified or split |
| 96 | +- Cyclomatic complexity > 10 requires justification |
| 97 | + |
| 98 | +**Naming:** |
| 99 | +- Descriptive names over abbreviations |
| 100 | +- No single-letter variables except: `i`, `j` for indices; `x`, `y` for coordinates; `f` for function parameters in HOFs |
| 101 | +- British English spelling in user-facing strings and docs |
| 102 | + |
| 103 | +**Formatting:** |
| 104 | +- Must pass project's formatter (rustfmt, ormolu, mix format, etc.) |
| 105 | +- No trailing whitespace |
| 106 | +- Files must end with single newline |
| 107 | +- UTF-8 encoding only |
| 108 | + |
| 109 | +--- |
| 110 | + |
| 111 | +### Security |
| 112 | + |
| 113 | +**Flag immediately:** |
| 114 | +- Hardcoded credentials, API keys, secrets |
| 115 | +- SQL string concatenation (injection risk) |
| 116 | +- Unsanitised user input in shell commands |
| 117 | +- Use of `eval` or equivalent in any language |
| 118 | +- Disabled TLS verification |
| 119 | +- Weak cryptographic choices (MD5, SHA1 for security) |
| 120 | + |
| 121 | +**Require justification:** |
| 122 | +- Any use of `unsafe` in Rust |
| 123 | +- Any FFI calls |
| 124 | +- Deserialisation of untrusted data |
| 125 | +- Network requests to non-HTTPS endpoints |
| 126 | + |
| 127 | +--- |
| 128 | + |
| 129 | +### Dependencies |
| 130 | + |
| 131 | +**Flag for review:** |
| 132 | +- Any new dependency — must justify why not stdlib |
| 133 | +- Dependencies with < 100 GitHub stars or < 1 year old |
| 134 | +- Dependencies without recent maintenance (> 1 year since release) |
| 135 | +- Transitive dependency count increase > 10 |
| 136 | + |
| 137 | +**Reject:** |
| 138 | +- Dependencies with known vulnerabilities |
| 139 | +- Dependencies with incompatible licences (GPL in MIT project, etc.) |
| 140 | +- Vendored code without licence attribution |
| 141 | + |
| 142 | +--- |
| 143 | + |
| 144 | +### Testing |
| 145 | + |
| 146 | +**Required:** |
| 147 | +- Unit tests for all public functions with logic |
| 148 | +- Integration tests for API boundaries |
| 149 | +- Property-based tests for parsers, serialisers, algorithms |
| 150 | + |
| 151 | +**Coverage:** |
| 152 | +- New code should not decrease coverage percentage |
| 153 | +- Critical paths require explicit test coverage |
| 154 | + |
| 155 | +**Flag if missing:** |
| 156 | +- Edge case tests (empty input, max values, unicode) |
| 157 | +- Error path tests (not just happy path) |
| 158 | + |
| 159 | +--- |
| 160 | + |
| 161 | +### Accessibility |
| 162 | + |
| 163 | +**User interfaces must:** |
| 164 | +- Support keyboard navigation |
| 165 | +- Have sufficient colour contrast (WCAG AA minimum) |
| 166 | +- Include alt text for images |
| 167 | +- Not rely solely on colour to convey information |
| 168 | +- Support screen readers where applicable |
| 169 | + |
| 170 | +**CLI tools must:** |
| 171 | +- Support `--help` and `--version` |
| 172 | +- Use stderr for errors, stdout for output |
| 173 | +- Return appropriate exit codes |
| 174 | +- Support `NO_COLOR` environment variable |
| 175 | + |
| 176 | +--- |
| 177 | + |
| 178 | +### Commits |
| 179 | + |
| 180 | +**Reject PRs with:** |
| 181 | +- Merge commits (rebase instead) |
| 182 | +- WIP commits that should be squashed |
| 183 | +- Commits mixing unrelated changes |
| 184 | +- Commit messages without clear description |
| 185 | + |
| 186 | +**Commit message format:** |
0 commit comments