Skip to content

Commit 0055f53

Browse files
committed
Address review comments on standards and review skill
standards.md: - Keyword-only arguments: rewritten around positional count — one or two for the data, config after the `*`, three+ is the smell. Now Recommended. - Changelog demoted to Nice; hand curation is real work, so point at towncrier/git-cliff instead of prescribing it. - One return type: show the function, so the None path is visible. - New rules: Tag every release, Type checking in CI. - Publish from a tag or a release, showing the `on: push: tags` trigger. - Test the matrix: CI is not free; test only what you claim to support. - Docs: name Quarto, Great Docs and zensical as alternatives; replace the robots.txt note with access-controlled Pages on GitHub Enterprise. - Makefile becomes A task runner, covering `just` for Windows. - Fix checkout@v3 -> v4 and a backwards `min`/`max` in the clip example. review-python-package skill: - Use Glob/Read/Grep instead of `ls`, `cat` and piped `grep` — the reviewer may be on Windows with no POSIX shell. Table-escaped pipes had also made the mutable-default pattern match nothing. - `uvx ruff`, and say so when the repo has no ruff config.
1 parent fba8826 commit 0055f53

2 files changed

Lines changed: 138 additions & 44 deletions

File tree

‎.claude/skills/review-python-package/SKILL.md‎

Lines changed: 41 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -27,37 +27,61 @@ Read the local file if this skill is running inside that repository
2727

2828
Work from what the repository actually contains. Do not guess at numbers you can measure.
2929

30-
Run these — fast, read-only, no side effects:
30+
The reviewer may be on Windows without a POSIX shell, so use the Glob, Read and Grep tools
31+
for anything that inspects files — never `ls`, `cat`, or a pipe into `grep`.
32+
33+
| What | How |
34+
| --- | --- |
35+
| Layout, `.gitignore`, `LICENSE`, `README` | Glob `*` and `.*` at the root |
36+
| Build system, metadata, dependencies, versioning | Read `pyproject.toml` |
37+
| CI | Glob `.github/workflows/*`, then Read each |
38+
| Data committed to git | `git ls-files "*.csv" "*.nc" "*.dfs*" "*.xlsx" "*.zip" "*.parquet"` |
39+
| Commit hygiene | `git log --oneline -20` |
40+
41+
Then run the linter — ground truth, not a guess. `uvx` so it works without the repository's
42+
environment installed:
3143

3244
```bash
33-
ls -a # layout, .gitignore, LICENSE, README
34-
cat pyproject.toml # build system, metadata, dependencies, versioning
35-
ls .github/workflows/ && cat .github/workflows/*.yml
36-
ruff check . # ground truth, not a guess
37-
ruff format --check .
38-
git log --oneline -20 # commit hygiene
39-
git ls-files | grep -Ei '\.(csv|nc|dfs.|xlsx|zip|parquet)$' # data in git
45+
uvx ruff check .
46+
uvx ruff format --check .
4047
```
4148

49+
`ruff` reads the repository's `[tool.ruff]` config if there is one. If there is none you are
50+
seeing the default ruleset, which is not the same as the project's intent — say so in the
51+
report rather than presenting the output as the project's own standard.
52+
4253
Then read the source: `src/` (or the package directory), `tests/`, `docs/`.
4354

4455
**Do not run** `pytest`, `mypy` or `uv sync` on your own initiative — they are slow, may need
4556
network or credentials, and an unfamiliar test suite may have side effects. Report what the
46-
test suite looks like and offer to run it.
57+
test suite looks like and offer to run it. Whether a type checker runs at all is visible in
58+
the workflow file — judge that from reading it, not by running one.
4759

4860
## Check for the code smells the course names
4961

50-
Grep as a starting point, then read the hits — a grep match is a candidate, not a finding.
62+
Search as a starting point, then read the hits — a match is a candidate, not a finding.
63+
64+
Use the Grep tool over `**/*.py`, showing line numbers. These are regular expressions for
65+
that tool, not shell commands — do not wrap them in `grep`, and do not escape the `|`.
66+
67+
```
68+
def .*=\s*(\[\]|\{\}|set\(\)|dict\(\)|list\(\)) # mutable default arguments
69+
except\s*: # bare except
70+
except.*:\s*pass # swallowed error, one-line form
71+
def [a-z]+[A-Z] # java-like API
72+
\w+\.\w*\._[a-z] # reaching into another object's internals
73+
```
74+
75+
`except.*:\s*pass` only catches the one-line form, so read every `except` block you find. For
76+
the last pattern, discard the `self._` hits — those are the class's own internals.
77+
78+
The rest have no pattern worth writing; read for them:
5179

52-
| Rule | Starting point |
80+
| Rule | What to look for |
5381
| --- | --- |
54-
| Mutable default arguments | `grep -rEn 'def .*=\s*(\[\]\|\{\}\|set\(\))' src/` |
5582
| Class variables that should be instance variables | mutable assignment in a class body, outside `__init__` |
5683
| Modified input arguments | assignment to a parameter's elements inside a function |
5784
| Mixed return types | multiple `return` statements of different types in one function |
58-
| Silently swallowed errors | `grep -rn 'except.*:\s*pass\|except:' src/` |
59-
| Java-like API | `grep -rEn 'def [a-z]+[A-Z]' src/` |
60-
| Reaching into another object's internals | `grep -rEn '\w+\.\w*\._[a-z]' src/` — exclude `self._` |
6185
| Missing docstrings | public functions and classes with no `"""` |
6286
| Removals with no deprecation path | `git log -p` on the public API vs `CHANGELOG.md` |
6387

@@ -68,7 +92,7 @@ Print the report in the conversation. Do not write a file unless asked.
6892
- One-line verdict first — is this a package someone can install and depend on, or not.
6993
- Then **Blockers**, **Recommended**, **Nice** in that order. Omit empty sections.
7094
- Every finding: `file:line` where there is one, what is wrong in one line, the fix, and the
71-
anchor URL of the rule.
95+
anchor URL of the rule. `✗` for Blockers, `⚠` for everything else.
7296
- Say what you checked and found clean — a short "✓ Packaging, docs, CI" line. Silence reads
7397
as "not checked".
7498
- Report what you did not check and why (tests not run, package not installed).
@@ -81,7 +105,7 @@ Blockers
81105
https://dhi.github.io/python-package-development/standards.html#pyproject.toml
82106
✗ No LICENSE — effectively all rights reserved, colleagues cannot legally use it
83107
https://dhi.github.io/python-package-development/standards.html#license
84-
⚠ src/clean.py:22 — mutable default `cart=[]` is shared across every call; use None
108+
✗ src/clean.py:22 — mutable default `cart=[]` is shared across every call; use None
85109
https://dhi.github.io/python-package-development/standards.html#mutable-default-arguments
86110
87111
Recommended

‎standards.md‎

Lines changed: 97 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -112,9 +112,15 @@ def old_function(x): ...
112112
catches uses of `@deprecated` at type-check time.
113113

114114
### Changelog
115-
*Recommended.* A `CHANGELOG.md` in [keepachangelog](https://keepachangelog.com/) format.
116-
Release notes written from a git log are not release notes — the reader wants to know what
117-
broke, what's new, and what's deprecated.
115+
*Nice.* A `CHANGELOG.md` in [keepachangelog](https://keepachangelog.com/) format. Release
116+
notes written from a git log are not release notes — the reader wants to know what broke,
117+
what's new, and what's deprecated.
118+
119+
Curating one by hand is real work, and a stale changelog is worse than none. Think twice
120+
before starting: if nobody reads it, skip it. If you do want one, let a tool assemble it from
121+
a fragment per pull request — [towncrier](https://towncrier.readthedocs.io/) or
122+
[git-cliff](https://git-cliff.org/) — so the cost lands on the author of each change rather
123+
than on you at release time.
118124

119125
### License
120126
*Blocker.* Without a license the package is "all rights reserved" and legally unusable by
@@ -209,18 +215,26 @@ Return a new object instead.
209215
```python
210216
def clip(values):
211217
for i in range(len(values)): # caller's list silently changed
212-
values[i] = min(0, values[i])
218+
values[i] = max(0, values[i])
213219

214220
def clip(values):
215-
return [min(0, v) for v in values] # ✓
221+
return [max(0, v) for v in values] # ✓
216222
```
217223

218224
### One return type
219225
*Blocker.* A function that returns a `bool` on success and a `str` on failure will read as
220226
success — a non-empty string is truthy.
221227

228+
A function with a `return` on one path and nothing on another is the same bug: the missing
229+
path returns `None`.
230+
222231
```python
223-
if is_operable(height=12.0, period=5.0): # returns "No way!" — and this runs
232+
def is_operable(height, period):
233+
if height > 10.0:
234+
return "No way!" # str here, None on every other path
235+
return True # ...and bool here
236+
237+
if is_operable(height=12.0, period=5.0): # "No way!" is truthy — this runs
224238
print("Go ahead!")
225239
```
226240

@@ -251,15 +265,24 @@ class Toolbox:
251265

252266
### Type hints
253267
*Recommended.* On public functions at minimum. They are hints, not enforcement — they exist
254-
for the reader and the editor.
268+
for the reader and the editor, until you add a [type checker](#type-checking-in-ci).
255269

256270
```python
257271
def clip(values: list[int], *, threshold: int = 0) -> list[int]: ...
258272
```
259273

260274
### Keyword-only arguments
261-
*Nice.* `def f(*, x, y)` forces callers to be explicit and lets you reorder parameters later
262-
without breaking anyone.
275+
*Recommended.* One or two positional parameters is fine — that is the data the function
276+
operates on. Everything after them is configuration, and belongs after a `*` so callers have
277+
to name it. You can then reorder or add options without breaking anyone.
278+
279+
Three or more positional parameters is a strong smell: `resample(df, 3, 0, True)` can't be
280+
read at the call site, and nobody can safely change the order again.
281+
282+
```python
283+
def resample(data, freq, *, offset=0, dropna=True): ...
284+
resample(df, "1h", dropna=False) # ✓ the data and its frequency; the rest is named
285+
```
263286

264287
### Dataclasses for data
265288
*Recommended.* Fields with type hints, a constructor, a useful `repr`, and equality by value
@@ -297,7 +320,8 @@ happening, rename something instead.
297320

298321
### When a long signature is a smell
299322
*Nice.* Many optional keyword arguments with sane defaults are perfectly Pythonic — see
300-
`read_csv`, `plot`, or any sklearn estimator. The smell is not the count; it's when the
323+
`read_csv`, `plot`, or any sklearn estimator. The smell is not the total count (that's
324+
[positional arguments](#keyword-only-arguments), which are a separate rule); it's when the
301325
arguments are switches for **separate jobs** the function has absorbed. If half the signature
302326
only applies when another argument is set, that's several functions wearing one signature.
303327

@@ -398,17 +422,22 @@ def remove_outlier(data: pd.DataFrame, column: str, threshold: float = 3) -> pd.
398422
```
399423

400424
### Examples that are tested
401-
*Nice.* `doctest` runs the examples in your docstrings. Documentation that is wrong is worse
402-
than documentation that is missing.
425+
*Nice.* Documentation that is wrong is worse than documentation that is missing. `doctest`
426+
runs the examples in your docstrings. For prose pages, [Quarto](https://quarto.org/) executes
427+
every snippet as part of the build, so the docs cannot ship broken — the build fails first.
403428

404429
```bash
405430
python -m doctest -v add.py
406431
```
407432

408433
### Published API documentation
409434
*Recommended.* `mkdocs` + `mkdocstrings` + GitHub Pages, at
410-
`https://dhi.github.io/<repository>/`. Note that a private repository can still have a public
411-
website — `robots.txt` hides it from search engines but is not security.
435+
`https://dhi.github.io/<repository>/`. [Quarto](https://quarto.org/),
436+
[Great Docs](https://github.com/machow/great-docs) (which wraps Quarto) and
437+
[zensical](https://zensical.org/) are viable alternatives.
438+
439+
A private repository can have access-controlled Pages on GitHub Enterprise — use that when the
440+
site should stay internal, rather than relying on the URL not being found.
412441

413442
## Automation
414443

@@ -425,36 +454,63 @@ jobs:
425454
test:
426455
runs-on: ubuntu-latest
427456
steps:
428-
- uses: actions/checkout@v3
457+
- uses: actions/checkout@v4
429458
- uses: astral-sh/setup-uv@v6
430459
with: { python-version: "3.13" }
431460
- run: uv sync
432461
- run: uv run pytest
433462
```
434463
435464
### Lint and format with ruff
436-
*Recommended.* `ruff check` finds unused imports, undefined names and dead variables — usually
437-
typos, sometimes bugs. `ruff format` ends style arguments. Run both in CI.
465+
*Recommended.* There is no reason not to. One binary, no configuration required, and it
466+
replaces flake8, black and isort at once. `ruff check` finds unused imports, undefined names
467+
and dead variables — usually typos, sometimes bugs. `ruff format` ends style arguments. Run
468+
both in CI.
438469

439470
```bash
440471
ruff check . ruff format --check .
441472
```
442473

443-
### Makefile
474+
### Type checking in CI
475+
*Nice.* [Type hints](#type-hints) are not enforcement — a type checker is. Run `mypy` (or
476+
`ty`) in CI on the package, not the tests, and turn it on for new code before old. It also
477+
catches uses of anything you have marked
478+
[`@deprecated`](#deprecate-before-removing).
479+
480+
```bash
481+
uv run mypy src --enable-error-code=deprecated
482+
```
483+
484+
### A task runner
444485
*Nice.* One source of truth for how to run the project's tools, and the fastest onboarding
445-
document there is.
486+
document there is. A `Makefile` if everyone is on Linux or macOS; `just` if anyone is on
487+
Windows, where `make` is not installed by default and `just` is a single binary
488+
(`uv tool install rust-just`).
446489

447490
```makefile
448-
check: lint test
491+
check: lint test # Makefile
492+
lint:
493+
uv run ruff check src
494+
test:
495+
uv run pytest
496+
```
497+
498+
```just
499+
check: lint test # justfile
449500
lint:
450-
ruff check src
501+
uv run ruff check src
451502
test:
452-
pytest
503+
uv run pytest
453504
```
454505

455506
### Test the matrix
456507
*Nice.* If you claim to support Windows and Python 3.10, test on Windows and Python 3.10.
457508

509+
Test what you claim and no more. CI is not free — every cell costs minutes on every push. An
510+
[application](#libraries-loose-applications-pinned) has one deployment target, so one cell is
511+
the honest matrix; a library that others install needs the range it advertises in
512+
`requires-python`.
513+
458514
```yaml
459515
strategy:
460516
matrix:
@@ -464,14 +520,28 @@ strategy:
464520

465521
## Release
466522

467-
### Publish from a GitHub release
468-
*Recommended.* Tag a release, let a workflow build and publish. Use Trusted Publishers so
469-
there are no secrets to manage.
523+
### Tag every release
524+
*Recommended.* An annotated `vX.Y.Z` tag, pushed. It is what makes "which commit is 1.2.0?"
525+
answerable a year later, and what lets you diff two releases. Just do it — it costs one
526+
command.
527+
528+
```bash
529+
git tag -a v1.2.0 -m "v1.2.0"
530+
git push --tags
531+
```
532+
533+
### Publish from a tag or a release
534+
*Recommended.* Let a workflow build and publish; never upload from your laptop. Use
535+
[Trusted Publishers](https://docs.pypi.org/trusted-publishers/) so there are no secrets to
536+
manage.
537+
538+
Trigger on the tag, or on a published GitHub release — either works with Trusted Publishers.
539+
The release gives you somewhere to put release notes; the tag is one step fewer.
470540

471541
```yaml
472542
on:
473-
release:
474-
types: [published]
543+
push:
544+
tags: ["v*"] # or: release: { types: [published] }
475545
```
476546

477547
### Somewhere to install from

0 commit comments

Comments
 (0)