-
Notifications
You must be signed in to change notification settings - Fork 169
Add code-review agent config from merged PR review patterns #439
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,130 @@ | ||
| # Copilot Code-Review Instructions — `clickhouse-rs` | ||
|
|
||
| This file configures the GitHub Copilot code-review agent for the | ||
| `ClickHouse/clickhouse-rs` repository (an async ClickHouse client for Rust). | ||
|
|
||
| The rules below were distilled from recurring maintainer feedback on merged pull | ||
| requests. When reviewing a change, check it against these rules and flag any | ||
| violations. Keep comments concrete and actionable, and cite the relevant rule. | ||
| Do not comment on pure style/formatting that `cargo fmt` and `cargo clippy` | ||
| already enforce — call those out only when CI would fail. | ||
|
|
||
| ## How to review | ||
|
|
||
| - Review effort level: medium — provide a balanced review that surfaces | ||
| substantive issues without exhaustive nitpicking. | ||
| - Prioritize correctness, public-API design, backwards compatibility, and | ||
| security over stylistic nits. | ||
| - Prefer the smallest change that fully addresses an issue. Apply YAGNI: do not | ||
| ask for, or approve, API surface that isn't needed yet. | ||
| - When a rule is violated, reference it (e.g. "See Naming & API design") and | ||
| suggest the concrete fix. | ||
|
|
||
| ## Naming & API design | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems to be valid for new methods added. should we move sections not related to code review to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it reads both, I would say so.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. the whole idea is that agent reads |
||
|
|
||
| - Use the `with_*` prefix for all public builder/setter methods on `Client`, | ||
| `Query`, `Insert`, and `Inserter` (e.g. `with_url`, `with_setting`, | ||
| `with_compression`). This keeps the configuration API discoverable. | ||
| - Prefer one general method over several narrow variants (e.g. a single | ||
| `with_roles(impl IntoIterator<Item = impl Into<String>>)` instead of | ||
| `with_role` + `with_default_roles` + `with_roles`). | ||
| - Boolean-flag `with_*` methods must take a `bool` argument rather than being | ||
| zero-argument toggles, so they compose with configuration structs | ||
| (`.with_validation(config.validate)`). | ||
| - Expose raw/unsafe bypass paths with an explicit `_unescaped` suffix (e.g. | ||
| `insert_unescaped()`); never auto-detect "magic" conditions (such as a dot in | ||
| a table name) to switch behavior. | ||
| - Name callback/hook combinators `with_<noun>_callback` (e.g. | ||
| `with_commit_callback`), not `with_on_<verb>`. | ||
| - Mark public enums that may gain variants — especially error enums — as | ||
| `#[non_exhaustive]`. | ||
| - Read accessors should take `&self` (not `&mut self`); use `get_*` / `set_*` | ||
| naming for inspection/mutation of options on `Client`. | ||
|
|
||
| ## SQL & query safety | ||
|
|
||
| - Never build SQL with `format!()` in library code or examples. Use ClickHouse | ||
| bind parameters (values and identifiers are escaped server-side). Examples | ||
| must teach the safe pattern. | ||
| - `Client::insert()` always escapes the table name. Pre-escaped or qualified | ||
| names (`db.table`) go through `insert_unescaped()`, where quoting is the | ||
| caller's responsibility. | ||
|
|
||
| ## Error handling | ||
|
|
||
| - Do not introduce a panic into a previously non-panicking, released code path. | ||
| A new panic is a breaking behavior change and must wait for the next major | ||
| version. | ||
|
Comment on lines
+55
to
+57
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This requires more nuance. If the panic is catching unsoundness or a programming error that would trigger a more harmful bug down the line, I'd rather have the panic. In general though, we should prefer returning errors rather than panicking. |
||
| - Add typed variants to `Error` instead of wrapping everything in | ||
| `Error::Other`. Remember each new variant is a SemVer hazard, so batch such | ||
| changes at a major-version boundary. | ||
|
Comment on lines
+58
to
+60
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Changing what error variant is emitted for a specific error condition (e.g. #426) is likely a breaking behavior change, though. |
||
| - Deserialize unstable external data (e.g. the `X-ClickHouse-Summary` header) | ||
| forward-compatibly: use `#[serde(default)]` and a `#[serde(flatten)]` | ||
| catch-all map rather than a rigid struct. | ||
|
|
||
| ## Documentation & changelog | ||
|
|
||
| - Every user-visible change needs a `CHANGELOG.md` entry under the correct | ||
| heading: `Added`, `Changed`, `Fixed`, or `Removed`. `Removed` is only for | ||
| deleted public APIs; internal-only removals go under `Changed`. | ||
|
Comment on lines
+67
to
+69
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Contributors tend to be hit-or-miss about writing good CHANGELOG entries so unless they choose to write one themselves, I just go in and add it during release. |
||
| - When renaming a public API, keep the old name with `#[deprecated]` and | ||
| document both the new method and the deprecation; schedule removal for a | ||
| future major version. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't know what action Copilot should be expected to take here, if any. Maybe |
||
| - All public APIs must have rustdoc; the crate will enforce `missing_docs`. | ||
| Use `[brackets]` for intra-doc links so the docs build does not break. | ||
| - Document important caveats inline on the API, not only in the changelog | ||
| (e.g. high Zstd levels block the async executor; `summary()` needs | ||
| `wait_end_of_query=1`). Prefer showing behavior with an example over prose. | ||
| - In examples, inline SQL strings and table names; don't extract them into | ||
| constants or helper variables that obfuscate the flow. | ||
|
|
||
| ## Testing | ||
|
|
||
| - New features and bug fixes require integration tests under `tests/it/`. | ||
| Tests exercising experimental server features must enable the required | ||
| settings via `.with_setting(...)` (e.g. | ||
| `.with_setting("allow_experimental_variant_type", "1")`). | ||
| - Before submitting, changes must pass `cargo test`, | ||
| `cargo test --no-default-features`, and `cargo test --all-features`, plus | ||
| `cargo fmt` and `cargo clippy --all-targets`. | ||
| - Performance-sensitive changes (de/serialization, transport, buffering, | ||
| tracing/observability) must include benchmark results from the `benches/` | ||
| suite. | ||
| - It is acceptable to omit tests for genuinely unreachable branches when | ||
| justified; do not add unreachable tests just to satisfy coverage. | ||
|
|
||
| ## Tracing & observability | ||
|
|
||
| - `tracing` span/event messages must be static strings; put dynamic data | ||
| (query text, error details) into span *fields* so events can be aggregated. | ||
| - Do not surface internal/private implementation failures at `WARN`/`ERROR`; | ||
| use `DEBUG`, and avoid noisy library-level `DEBUG` events that users would | ||
| have to suppress (e.g. expected retry/overload errors). | ||
|
|
||
| ## Dependencies & feature flags | ||
|
|
||
| - External contributors must not upgrade dependencies without prior maintainer | ||
| discussion (security policy in `CONTRIBUTING.md`). | ||
| - Use weak feature dependencies (`dep?/feature`, e.g. `arrow-ipc?/lz4`) so a | ||
| sub-dependency is not pulled in unless its own feature is enabled. | ||
| - Keep public-API dependencies that release breaking versions on a fast cadence | ||
| (e.g. `arrow`) in a separately versioned sub-crate, not in the core | ||
| `clickhouse` crate. | ||
| - Remove abandoned or unused (dev-)dependencies to keep the graph clean. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like this could tend towards unrelated changes slipping into PRs. |
||
|
|
||
| ## Backwards compatibility | ||
|
|
||
| - Never silently remove or rename public API in a minor/patch release — | ||
| deprecate first, remove at the next major version. | ||
| - Treat panicking-behavior changes as breaking and defer them to the next major | ||
| version. | ||
| - Avoid arbitrary hard-coded upper limits (block sizes, compression levels, | ||
| etc.) unless ClickHouse itself imposes them; such limits become footguns for | ||
| legitimate use cases. | ||
|
|
||
| ## Examples | ||
|
|
||
| - Add runnable examples under `examples/` and update `examples/README.md` for | ||
| major new features. | ||
| - Place format/integration examples (e.g. Arrow) under `Special cases` in | ||
| `examples/README.md`, not in the ClickHouse data-types section. | ||
Uh oh!
There was an error while loading. Please reload this page.