Add distilled package standards page and review skill by ecomodeller · Pull Request #41 · DHI/python-package-development

ecomodeller · 2026-08-06T14:48:14Z

The course is seven slide decks, which makes it awkward to point someone at a specific rule mid-conversation. standards.md distils it into ~45 normative rules, one linkable heading each, published at standards.html:

…/standards.html#mutable-default-arguments
…/standards.html#composition-over-inheritance
…/standards.html#every-dependency-is-a-decision

Only rules you'd assert at someone — the exposition stays in the slides. That includes rules no tool can check (keep PRs small, composition over inheritance), since the point is lecturing, not just linting.

Issues folded in

Seven that never made it into the slides:

Closes Module 02: cover the underscore-prefix 'private/internal' convention #40 — the underscore convention, including the amendment that issue asks for: breaking changes apply to the public API only, cross-linked both ways
Closes Add slides on the cost of adding dependencies #37 — cost of adding a dependency
Closes Add fluent interface pattern as teaching example #36 — long signatures, framed as "the function absorbed several jobs" rather than a parameter count, since many optional kwargs with sane defaults is idiomatic
Closes Add section on inappropriate intimacy between classes #35 — reaching into another class's internals
Closes Add Ousterhout's comment philosophy to course content #34 — comments say why, not what
Closes Add content on deprecating functionality #33, closes Deprecations #26 — deprecation, merged into one rule
Closes Changelog #22 — changelog

Left open deliberately: #39 (great-docs is still "worth evaluating"), #31 and #32 (pedagogy, belongs in the slides), #25 and #24 (questions you haven't settled).

Review skill

.claude/skills/review-python-package/ audits a package repo against the page and links every finding to the rule it breaks, so the report is something you can paste into a PR and the reader gets the explanation without you there. It runs ruff for ground truth but deliberately won't run pytest/mypy/uv sync unprompted on an unfamiliar repo.

The finding URLs 404 until this merges and Pages rebuilds.

The course content is spread across seven slide decks, which makes it hard to point someone at a specific rule. standards.md distils it into ~45 normative rules, one linkable heading each, published at standards.html. Exposition is deliberately left in the slides: the page holds only rules you would assert at someone, including ones no tool can check. Also folds in seven issues that never made it into the slides: #40 (the underscore convention, with the public-API-only amendment to breaking changes), #37 (cost of dependencies), #36 (long signatures), #35 (inappropriate intimacy), #34 (comments say why), #33/#26 (deprecation), #22 (changelog). The review-python-package skill audits a repository against the page and links each finding to the rule it breaks.

ecomodeller · 2026-08-06T17:06:25Z

+`FutureWarning` is for end users (always visible). `mypy --enable-error-code=deprecated`
+catches uses of `@deprecated` at type-check time.
+
+### Changelog


This is nice to have, but is tedious to curate manually. Think twice before spending time on this.

Agreed — demoted to Nice in 0055f53, with the cost stated plainly ("a stale changelog is worse than none; if nobody reads it, skip it"). Rather than drop the rule, it now points at towncrier / git-cliff so the work lands on the author of each PR instead of on you at release time.

ecomodeller · 2026-08-06T17:08:08Z

+success — a non-empty string is truthy.
+
+```python
+if is_operable(height=12.0, period=5.0):   # returns "No way!" — and this runs


This function returns None. Fix it.

Fixed in 0055f53 — the example only had the call site, so nothing showed the mixed return. It now defines is_operable, with the str / None / bool paths visible, plus a line making the implicit-None case explicit as the same bug.

I read this as being about the One return type example. If you meant the clip above it, that one returns None on purpose — it is the bad variant showing mutation — so say the word and I will handle it differently.

ecomodeller · 2026-08-06T17:09:27Z

+```
+
+### Keyword-only arguments
+*Nice.* `def f(*, x, y)` forces callers to be explicit and lets you reorder parameters later


This should be more nuanced. One or two positional arguments are fine, that is usually the data the function is operating upon. More than two positional arguments are a strong code smell.

Adopted your wording in 0055f53 and promoted the rule to Recommended. It now says one or two positional parameters is fine (the data being operated on), everything after belongs behind the *, and three or more positionals is a strong smell — resample(df, 3, 0, True) as the counter-example.

This collided with When a long signature is a smell, which said "the smell is not the count". Scoped that one to the total count and cross-linked the two so the page does not argue with itself.

ecomodeller · 2026-08-06T17:11:18Z

+```
+
+### Examples that are tested
+*Nice.* `doctest` runs the examples in your docstrings. Documentation that is wrong is worse


Using Quarto for documentation, where the snippets are run as part of the build is another option.

Added in 0055f53 — doctest for docstring examples, Quarto for prose pages where the build executes every snippet and fails first.

ecomodeller · 2026-08-06T17:12:18Z

+```
+
+### Published API documentation
+*Recommended.* `mkdocs` + `mkdocstrings` + GitHub Pages, at


Quarto or the new framework Great Docs (wrapping Quarto), or zensical are all viable options.

Added in 0055f53 — mkdocs + mkdocstrings stays the default, with Quarto, Great Docs and zensical named as viable alternatives (all linked).

Worth deciding whether #39 stays open now that the page names Great Docs.

ecomodeller · 2026-08-06T17:12:39Z

+### Published API documentation
+*Recommended.* `mkdocs` + `mkdocstrings` + GitHub Pages, at
+`https://dhi.github.io/<repository>/`. Note that a private repository can still have a public
+website — `robots.txt` hides it from search engines but is not security.


No longer relevant. With GitHub Enterprise, we can have private pages.

Removed in 0055f53. The robots.txt sentence is gone; the rule now says a private repository can have access-controlled Pages on GitHub Enterprise, and to use that rather than relying on the URL not being found.

ecomodeller · 2026-08-06T17:13:19Z

+```
+
+### Lint and format with ruff
+*Recommended.* `ruff check` finds unused imports, undefined names and dead variables — usually


There is no reason not use ruff for linting and formatting.

Your sentence is now the opening of the rule in 0055f53, with the reason spelled out: one binary, no configuration required, replaces flake8 + black + isort.

I left the severity at Recommended rather than promoting it. Blocker is defined at the top of the page as "not fit to share", and an unlinted package is still shareable — it would be the first rule to stretch that definition. Happy to promote it if you would rather the legend bend.

ecomodeller · 2026-08-06T17:13:51Z

+```
+
+### Makefile
+*Nice.* One source of truth for how to run the project's tools, and the fastest onboarding


Consider just instead of make, more modern and better cross platform support.

Done in 0055f53 — the heading is now A task runner: Makefile if everyone is on Linux or macOS, just if anyone is on Windows, where make is absent by default and just is a single binary (uv tool install rust-just). Both recipe files are shown, both using uv run. Anchor changed to #a-task-runner; nothing linked to the old one.

ecomodeller · 2026-08-06T17:15:17Z

+strategy:
+  matrix:
+    os: [ubuntu-latest, windows-latest]
+    python-version: ["3.10", "3.13"]


Matrix testing is powerful, but don't go overboard. If your package is actually an application, targeting a single Python version might be ok. CI is great, but it is not free.

Added in 0055f53 — "test what you claim and no more", CI is not free, every cell costs minutes on every push. Uses the library/application split the page already makes, cross-linked: an application has one deployment target, so one cell is the honest matrix.

ecomodeller · 2026-08-06T17:16:52Z

+## Release
+
+### Publish from a GitHub release
+*Recommended.* Tag a release, let a workflow build and publish. Use Trusted Publishers so


Using git tags to track released versions makes your life easy. Just do it.

Promoted to its own rule in 0055f53 — Tag every release, Recommended, annotated vX.Y.Z pushed, framed as what makes "which commit is 1.2.0?" answerable a year later. Two commands, so "just do it" is in there.

ecomodeller · 2026-08-06T17:17:24Z

+
+```yaml
+on:
+  release:


A viable alternative is to trigger automated releases via git tags.

Added in 0055f53 — the rule is now Publish from a tag or a release and shows on: push: tags: ["v*"], with the release trigger as the inline alternative and one clause on the tradeoff: the release gives you somewhere for release notes, the tag is one step fewer. Trusted Publishers works with either.

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.

ecomodeller commented Aug 6, 2026

View reviewed changes

Uh oh!

Conversation

ecomodeller commented Aug 6, 2026

Issues folded in

Review skill

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant