[DBA-282] Replace generic coding guidelines with Python-specific doc#67
[DBA-282] Replace generic coding guidelines with Python-specific doc#67
Conversation
Strip filler advice from docs/coding-guidelines.md and rewrite as docs/python-coding-guidelines.md — grounded in actual databao-cli patterns with code examples. Filename scoped to Python so future TypeScript guidelines slot in cleanly. Update all references. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Replaces the repo’s generic coding guidelines document with a Python-specific guidelines doc for databao-cli, and updates all internal references and validation hooks to point at the new filename.
Changes:
- Added
docs/python-coding-guidelines.mdand removeddocs/coding-guidelines.md. - Updated references in agent guidance docs and repo metadata files to the new doc path.
- Updated smoke-test and pre-commit file filters to validate the new guidelines doc.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/smoke-test-skills.sh | Updates smoke check to assert the new guidelines doc exists. |
| docs/python-coding-guidelines.md | Introduces new Python-specific project coding guidelines and examples. |
| docs/coding-guidelines.md | Deletes the old generic coding guidelines doc. |
| CLAUDE.md | Updates reference link to point at the new guidelines doc. |
| .pre-commit-config.yaml | Updates validate-agent-guidance hook file regex to include the new doc name. |
| .claude/skills/review-architecture/SKILL.md | Updates the “review order” list to reference the new guidelines doc. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Python Coding Guidelines | ||
|
|
||
| Rules for writing Python in `databao-cli`. Covers patterns the linter can't | ||
| enforce. For style/formatting, run `ruff check --fix && ruff format` and move on. |
There was a problem hiding this comment.
The doc recommends running ruff check --fix && ruff format, but earlier in this same doc it states that repo commands should run via uv run. To keep this consistent with the project tooling (and avoid running a different ruff than the one pinned in the uv env), update the command examples here to use uv run ruff ... (or point to make check).
| enforce. For style/formatting, run `ruff check --fix && ruff format` and move on. | |
| enforce. For style/formatting, run `uv run ruff check --fix && uv run ruff format` and move on. |
| - Errors from bad input: `click.UsageError`. Errors from broken state: `sys.exit(1)` | ||
| after a human-readable `click.echo(..., err=True)`. |
There was a problem hiding this comment.
This guideline says to use click.UsageError for bad input, but there are no usages of click.UsageError anywhere in the codebase (current commands typically click.echo(..., err=True) then sys.exit(1)). Either adjust the guideline to match the current project pattern, or update the CLI commands to consistently raise click.UsageError so Click can format errors uniformly.
| - Errors from bad input: `click.UsageError`. Errors from broken state: `sys.exit(1)` | |
| after a human-readable `click.echo(..., err=True)`. | |
| - For user-facing errors (bad input or broken state), emit a clear | |
| `click.echo(..., err=True)` message and then `sys.exit(1)`. |
| request_id = str(uuid6()) | ||
| try: | ||
| ... | ||
| return json.dumps(result, default=str) |
There was a problem hiding this comment.
The doc states “Don’t blanket try/except” and later lists except Exception as something to avoid without re-raise, but this MCP tool example catches Exception and returns an error JSON. If the intent is to allow broad exception catching at API boundaries (MCP tool wrappers) to prevent server crashes, call that out explicitly here; otherwise, narrow the exception type(s) in the example to align with the earlier guidance.
| return json.dumps(result, default=str) | |
| return json.dumps(result, default=str) | |
| # Broad catch is acceptable here at the MCP tool boundary so we can | |
| # always return a structured error JSON instead of crashing the server. | |
| # Inside the codebase, avoid blanket `except Exception` without re-raise. |
| # review-architecture: referenced doc files exist and are non-empty | ||
| for doc in docs/architecture.md docs/coding-guidelines.md docs/testing-strategy.md README.md; do | ||
| for doc in docs/architecture.md docs/python-coding-guidelines.md docs/testing-strategy.md README.md; do | ||
| run_test "review-architecture: $doc exists and non-empty" "test -s $doc" |
There was a problem hiding this comment.
test -s $doc is unquoted inside an eval string. Even though these paths are currently simple, quoting the variable (test -s "$doc") avoids word-splitting/globbing issues and makes the helper more robust if the list ever includes paths with spaces or special chars.
| run_test "review-architecture: $doc exists and non-empty" "test -s $doc" | |
| run_test "review-architecture: $doc exists and non-empty" "test -s \"$doc\"" |
Summary
Replace the generic
docs/coding-guidelines.mdwithdocs/python-coding-guidelines.md— project-specific Python patterns with code examples, stripped of filler. Filename scoped to Python for future TypeScript guidelines.Changes
New Python coding guidelines
Files
docs/python-coding-guidelines.md(new)docs/coding-guidelines.md(deleted)Reference updates
Files
CLAUDE.md.pre-commit-config.yamlscripts/smoke-test-skills.sh.claude/skills/review-architecture/SKILL.mdTest Plan
make checkpasses (ruff + mypy + validate-agent-guidance)make testpasses (65/65)make lint-skillspassesmake smoke-skills— all review-architecture doc checks pass🤖 Generated with Claude Code