From 8ab520ce4cec1085805744af462a55fbffbaf7fd Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Sat, 16 May 2026 06:08:47 +0000 Subject: [PATCH 1/2] docs: expand REVIEW.md with conventions from commit history Co-Authored-By: Sachin Iyer --- REVIEW.md | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 107 insertions(+), 1 deletion(-) diff --git a/REVIEW.md b/REVIEW.md index 9cccecc..8f08f2c 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -1,4 +1,110 @@ # Review Guidelines -## Conventions +Rules for reviewing pull requests to this repository. Each bullet is a +convention derived from the project's commit history and enforced lint +configuration. + +## Visibility + - Do not use `pub(super)`. Use `pub(crate)` or `pub` instead. +- Prefer the narrowest visibility that compiles. If a function is only used + within its own module, plain `fn` is better than `pub(crate)`. + +## Error handling + +- Never use `.unwrap()`, `.expect()`, or `panic!()` in production code (all + three are denied by clippy). In test code (`#[cfg(test)]`) they are fine. +- Use `anyhow::Result` as the return type and attach context with + `.context("…")` so errors propagate with a human-readable chain. +- Surface user-friendly error messages. `main.rs` prints `Error: {err:#}` (the + alternate Display chain) to stderr — keep that chain informative. +- Do not use `dbg!()`, `todo!()`, or `unimplemented!()` — all are denied. + +## Type safety & casts + +- No `as` casts — `clippy::as_conversions` is denied. Use `try_from` / + `try_into`, `.saturating_*()`, or `.div_ceil()` instead. +- No `unsafe` code — `unsafe_code` is denied at the Rust lint level. +- Prefer `const fn` where possible. +- Match arms on enums must be exhaustive — `wildcard_enum_match_arm` is denied. + Do not use `_ =>` catch-alls on enums. + +## Imports & paths + +- Use `use` imports at the top of the file. Do not write inline fully-qualified + paths — `clippy::absolute_paths` is denied. +- Do not use `println!` / `eprintln!` — `clippy::print_stdout` and + `clippy::print_stderr` are denied in `lib.rs`. Use `console::Term` for all + terminal output. + +## Generated code & OpenAPI + +- API types come from OpenAPI codegen via progenitor. Do not hand-write types + that duplicate generated ones. +- `src/api/types.rs` should contain only re-exports, type aliases, and trait + impls (`ValueEnum`, `Formattable`) on the generated types. +- Update the vendored `openapi.json` via `cargo xtask generate-openapi`, not by + hand. CI checks that the vendored spec matches upstream. +- Update CLI help docs via `cargo xtask generate-help > docs/HELP.md`. + +## Output format discipline + +- `--format json` must produce valid JSON. Never mix human-readable text (hints, + empty-result messages, update notices) into JSON output. Guard those messages + behind a format check. +- Auto-update notices and progress output must be suppressed when the command is + in "silent" mode (JSON output or `completions`). +- List views use a card-based layout, not raw tables. +- Use `console::Term` for terminal output, not `println!` / `eprintln!`. + +## Config file handling + +- Writes to `config.toml` must go through `update_config(|c| …)`, which holds + an exclusive file lock and preserves comments, formatting, and unknown + user-added keys. +- Never truncate-and-rewrite the config without locking — concurrent CLI + invocations will corrupt it. +- When a field is set to `None`, the key must be removed from disk (not left as + an empty value). + +## Concurrency + +- Use file locking (`fs2`) for any shared mutable file (config, update lock). +- Auto-update uses a non-blocking try-lock so concurrent CLI invocations skip + rather than stack up. +- Manual `detail update` uses a blocking lock so the user's explicit request + waits instead of silently skipping. + +## Testing + +- Unit tests live in `#[cfg(test)] mod tests` at the bottom of each source + file. Integration tests live in `tests/integration.rs`. +- Test edge cases: zero values, empty strings, non-ASCII input, negative + timestamps, concurrent writes, etc. +- Do not write brittle tests that depend on exact formatting or timing. +- The `with_temp_config` helper isolates config tests via a temporary + `XDG_CONFIG_HOME` — use it for any test that touches the config file. + +## CLI argument design + +- Validate inputs at the CLI boundary using clap `value_parser` ranges (e.g. + `1..=100` for `--limit`, `1..` for `--page`). +- The `repo` positional argument should be optional when possible — the CLI + infers `owner/repo` from the git remote `origin` when omitted. +- Use `serde(rename_all = "…")` on enums/structs rather than per-field + `#[serde(rename)]` attributes. +- Use `serde(default)` on config structs for forward-compatibility with new + fields. + +## Code organization + +- Shared helpers go in `src/utils/` (datetime, pagination, repos, git). + Extract common logic rather than duplicating it across commands. +- Use `LazyLock` for expensive static initializations (e.g. `MadSkin`). +- Remove dead code, dead feature gates, and unused functions promptly. + +## Commit messages + +- Follow conventional commits: `type(scope): description`. +- Common types: `feat`, `fix`, `refactor`, `chore`, `test`, `lint`, `docs`, + `ci`, `style`. From 74c602d7ecf208bda1bcb21547e8634ed301f2d5 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Sat, 16 May 2026 06:12:07 +0000 Subject: [PATCH 2/2] docs: slim REVIEW.md to non-lint-enforced conventions only Co-Authored-By: Sachin Iyer --- REVIEW.md | 114 ++++++++++-------------------------------------------- 1 file changed, 20 insertions(+), 94 deletions(-) diff --git a/REVIEW.md b/REVIEW.md index 8f08f2c..d66f157 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -1,107 +1,33 @@ # Review Guidelines -Rules for reviewing pull requests to this repository. Each bullet is a -convention derived from the project's commit history and enforced lint -configuration. +Conventions that require human judgment during review. Lint-enforced rules +(see `Cargo.toml` `[lints]` and `lib.rs` deny attributes) are omitted here. -## Visibility +## Conventions - Do not use `pub(super)`. Use `pub(crate)` or `pub` instead. -- Prefer the narrowest visibility that compiles. If a function is only used - within its own module, plain `fn` is better than `pub(crate)`. - -## Error handling - -- Never use `.unwrap()`, `.expect()`, or `panic!()` in production code (all - three are denied by clippy). In test code (`#[cfg(test)]`) they are fine. -- Use `anyhow::Result` as the return type and attach context with - `.context("…")` so errors propagate with a human-readable chain. -- Surface user-friendly error messages. `main.rs` prints `Error: {err:#}` (the - alternate Display chain) to stderr — keep that chain informative. -- Do not use `dbg!()`, `todo!()`, or `unimplemented!()` — all are denied. - -## Type safety & casts - -- No `as` casts — `clippy::as_conversions` is denied. Use `try_from` / - `try_into`, `.saturating_*()`, or `.div_ceil()` instead. -- No `unsafe` code — `unsafe_code` is denied at the Rust lint level. -- Prefer `const fn` where possible. -- Match arms on enums must be exhaustive — `wildcard_enum_match_arm` is denied. - Do not use `_ =>` catch-alls on enums. - -## Imports & paths - -- Use `use` imports at the top of the file. Do not write inline fully-qualified - paths — `clippy::absolute_paths` is denied. -- Do not use `println!` / `eprintln!` — `clippy::print_stdout` and - `clippy::print_stderr` are denied in `lib.rs`. Use `console::Term` for all - terminal output. - -## Generated code & OpenAPI - -- API types come from OpenAPI codegen via progenitor. Do not hand-write types - that duplicate generated ones. -- `src/api/types.rs` should contain only re-exports, type aliases, and trait - impls (`ValueEnum`, `Formattable`) on the generated types. -- Update the vendored `openapi.json` via `cargo xtask generate-openapi`, not by - hand. CI checks that the vendored spec matches upstream. -- Update CLI help docs via `cargo xtask generate-help > docs/HELP.md`. - -## Output format discipline - +- Prefer the narrowest visibility that compiles — plain `fn` over `pub(crate)` + when the function is only used within its own module. +- Use `anyhow::Result` with `.context("…")` so the error chain shown to users + via `Error: {err:#}` stays informative. - `--format json` must produce valid JSON. Never mix human-readable text (hints, - empty-result messages, update notices) into JSON output. Guard those messages - behind a format check. -- Auto-update notices and progress output must be suppressed when the command is - in "silent" mode (JSON output or `completions`). -- List views use a card-based layout, not raw tables. -- Use `console::Term` for terminal output, not `println!` / `eprintln!`. - -## Config file handling - -- Writes to `config.toml` must go through `update_config(|c| …)`, which holds - an exclusive file lock and preserves comments, formatting, and unknown - user-added keys. -- Never truncate-and-rewrite the config without locking — concurrent CLI - invocations will corrupt it. -- When a field is set to `None`, the key must be removed from disk (not left as - an empty value). - -## Concurrency + update notices) into JSON output — guard behind a format check. +- New commands that accept a `repo` positional should make it optional and fall + back to inferring `owner/repo` from the git remote. -- Use file locking (`fs2`) for any shared mutable file (config, update lock). -- Auto-update uses a non-blocking try-lock so concurrent CLI invocations skip - rather than stack up. -- Manual `detail update` uses a blocking lock so the user's explicit request - waits instead of silently skipping. +## Generated code -## Testing +- API types come from OpenAPI codegen (progenitor). Do not hand-write types that + duplicate generated ones. `src/api/types.rs` should only contain re-exports, + type aliases, and trait impls on generated types. +- Update `openapi.json` via `cargo xtask generate-openapi` and help docs via + `cargo xtask generate-help > docs/HELP.md`. Do not edit these by hand. -- Unit tests live in `#[cfg(test)] mod tests` at the bottom of each source - file. Integration tests live in `tests/integration.rs`. -- Test edge cases: zero values, empty strings, non-ASCII input, negative - timestamps, concurrent writes, etc. -- Do not write brittle tests that depend on exact formatting or timing. -- The `with_temp_config` helper isolates config tests via a temporary - `XDG_CONFIG_HOME` — use it for any test that touches the config file. - -## CLI argument design - -- Validate inputs at the CLI boundary using clap `value_parser` ranges (e.g. - `1..=100` for `--limit`, `1..` for `--page`). -- The `repo` positional argument should be optional when possible — the CLI - infers `owner/repo` from the git remote `origin` when omitted. -- Use `serde(rename_all = "…")` on enums/structs rather than per-field - `#[serde(rename)]` attributes. -- Use `serde(default)` on config structs for forward-compatibility with new - fields. - -## Code organization +## Config file handling -- Shared helpers go in `src/utils/` (datetime, pagination, repos, git). - Extract common logic rather than duplicating it across commands. -- Use `LazyLock` for expensive static initializations (e.g. `MadSkin`). -- Remove dead code, dead feature gates, and unused functions promptly. +- All writes to `config.toml` must go through `update_config(|c| …)`, which + holds a file lock and preserves user comments and unknown keys. +- When a config field is set to `None`, remove the key from disk. ## Commit messages