[Chore] add coding standard and PR review instructions#3039
[Chore] add coding standard and PR review instructions#3039deng451e merged 2 commits intoLMCache:devfrom
Conversation
Signed-off-by: ApostaC <yihua98@uchicago.edu>
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive coding standards and automated review skills for the LMCache project, including the addition of docs/coding_standards.md, CLAUDE.md, and specialized Claude skills for PR reviews and pre-PR checks. The feedback identifies opportunities to refine the allowed-tools list for the review skill, clarify the guidance on avoiding Optional in favor of union types, and allow for more specific exception types beyond ValueError for runtime validation.
| --- | ||
| name: pr-review | ||
| description: Review a GitHub pull request against the LMCache coding standards | ||
| allowed-tools: Bash, Read, Grep, Glob, Agent, mcp__github__pull_request_read, mcp__github__list_pull_requests, mcp__github__search_pull_requests, mcp__github__add_reply_to_pull_request_comment |
There was a problem hiding this comment.
The allowed-tools list includes mcp__github__add_reply_to_pull_request_comment but appears to be missing tools for creating new top-level comments or line-level review comments (e.g., mcp__github__add_pull_request_comment or mcp__github__create_pull_request_review_comment). If the agent is intended to post its review findings directly to GitHub rather than just outputting them to the user, these additional tools may be necessary.
| - Use `list[SomeType]`, not bare `list`. | ||
| - Use `dict[KeyType, ValueType]`, not bare `dict`. | ||
| - Use `tuple[int, str]` or `tuple[int, ...]`, not bare `tuple`. | ||
| - **Avoid `Optional`**. Prefer always initializing objects (even if empty/unused) over wrapping in `Optional`. If a value can genuinely be absent, use `X | None` syntax rather than `Optional[X]`. |
There was a problem hiding this comment.
The phrasing "Avoid Optional" is slightly ambiguous. It would be clearer to specify that the typing.Optional type hint should be avoided in favor of the | None union syntax (PEP 604), and that the use of nullability itself should be minimized by preferring default values or empty objects (Null Object Pattern) where appropriate. The current text correctly mentions the syntax but the heading could be more precise.
|
|
||
| ### 2.2 Runtime Validation | ||
|
|
||
| - **Never use `assert` for runtime-dependent checks** (e.g., config validation, input validation). Assertions are stripped by `python -O`. Use explicit `if ...: raise ValueError(...)` instead. |
There was a problem hiding this comment.
Mandating ValueError for all runtime-dependent checks is too restrictive. While ValueError is appropriate for invalid argument values, other exceptions like TypeError (for incorrect types), KeyError (for missing configuration keys), or custom domain-specific exceptions should be used where they more accurately describe the failure. This improves error handling and debuggability for callers.
Signed-off-by: ApostaC <yihua98@uchicago.edu>
* [Add] coding standard and ai skills Signed-off-by: ApostaC <yihua98@uchicago.edu> * skill for create PR Signed-off-by: ApostaC <yihua98@uchicago.edu> --------- Signed-off-by: ApostaC <yihua98@uchicago.edu>
What this PR does / why we need it:
Consolidates the LMCache coding quality and review standards into a single
authoritative document (
docs/coding_standards.md) and adds two Claude Codeskills that reference it, so authors and reviewers have a shared, actionable
playbook.
The standards doc synthesizes:
AGENTS.mdchecklist.gemini/styleguide.mdreview philosophyassertvsif/raise,missing tests as error-severity, log-level discipline, docstring accuracy,
caller-impact analysis)
Changes in this PR:
docs/coding_standards.md(new): sections on typing, docstrings, functionand interface design, new functionality, modification to existing code
(including caller impact / global view), code organization, thread
safety, and the review process with severity calibration.
CLAUDE.md(new): high-level pointers into the standards doc for theClaude Code harness.
.claude/skills/pr-review/SKILL.md(new): reviews a GitHub PR against thestandards, including a caller-impact step that checks unchanged callers of
modified symbols.
.claude/skills/pre-pr-check/SKILL.md(new): checks local changes before aPR is opened, auto-fixes mechanical issues (typing,
Optional,assert→if/raise, imports, logging), and flags design/test concerns for manualreview.
.gemini/styleguide.md: points to the new canonical doc..gitignore: removesCLAUDE.mdso agent instructions can be tracked.Special notes for your reviewers:
No code or runtime behavior changes in this PR -- docs and agent configs
only. The
.claude/skills/directory is a new location; per-developer localstate (
.claude/settings.local.json,.claude/worktrees/) stays ignored via.git/info/exclude.If applicable:
Note
Low Risk
Docs and agent-skill configuration only; no runtime code paths or APIs are modified.
Overview
Adds an authoritative
docs/coding_standards.mdcovering typing/docstrings/interface design, test expectations, logging/style rules, and a review process (including caller-impact analysis and severity calibration).Introduces Claude Code automation docs/config: new
CLAUDE.mdplus.claude/skills/workflows for/create-pr,/pre-pr-check(local standards checking with optional auto-fixes), and/pr-review(PR review checklist driven by the standards). Updates.gemini/styleguide.mdto point at the new standard and adjusts.gitignoreto trackCLAUDE.mdby no longer ignoring it.Reviewed by Cursor Bugbot for commit bba9f19. Bugbot is set up for automated code reviews on this repo. Configure here.