Add code-review agent config from merged PR review patterns#439
Add code-review agent config from merged PR review patterns#439Copilot wants to merge 2 commits into
Conversation
peter-leonov-ch
requested review from
abonander,
joe-clickhouse,
kavirajk and
peter-leonov-ch
as code owners
|
|
There was a problem hiding this comment.
Pull request overview
Adds/introduces a GitHub Copilot code-review instruction document for clickhouse-rs, including an explicit “Review effort level: medium” directive intended to keep reviews focused on substantive issues.
Changes:
- Added
.github/copilot-instructions.mdwith detailed code-review guidance for Copilot. - Included a “Review effort level: medium” bullet at the top of the “How to review” section.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
peter-leonov-ch
changed the title
| - When a rule is violated, reference it (e.g. "See Naming & API design") and | ||
| suggest the concrete fix. | ||
|
|
||
| ## Naming & API design |
There was a problem hiding this comment.
seems to be valid for new methods added. should we move sections not related to code review to AGENT.MD?
There was a problem hiding this comment.
If it reads both, I would say so.
There was a problem hiding this comment.
yes. the whole idea is that agent reads AGENT.md for development instructions
| - 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. |
There was a problem hiding this comment.
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.
| - 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`. |
There was a problem hiding this comment.
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.
| deleted public APIs; internal-only removals go under `Changed`. | ||
| - 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. |
There was a problem hiding this comment.
schedule removal for a future major version
I don't know what action Copilot should be expected to take here, if any. Maybe recommend opening an issue about removing the old method in a future version or something like that.
| - 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. |
There was a problem hiding this comment.
I feel like this could tend towards unrelated changes slipping into PRs.
| - 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. |
There was a problem hiding this comment.
The Error enum is #[non_exhaustive] so adding new variants is backwards-compatible. What is a potential SemVer hazard is adding new variants wrapping errors from external crates that weren't already treated as public dependencies. In that case, Error::Other is the safer path because we can hide behind the type-erasure.
Changing what error variant is emitted for a specific error condition (e.g. #426) is likely a breaking behavior change, though.
Summary
Adds an explicit "Review effort level: medium" directive to the Copilot code-review agent configuration so reviews surface substantive issues without exhaustive nitpicking.
Changes
Review effort level: mediumbullet to the top of the "How to review" section in.github/copilot-instructions.md.Checklist