From fba8826fbbae3ddfd0949ec0ba0d08bf54b6ea21 Mon Sep 17 00:00:00 2001 From: Henrik Andersson Date: Thu, 6 Aug 2026 16:47:53 +0200 Subject: [PATCH 1/5] Add distilled package standards page and review skill 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. --- .claude/skills/review-python-package/SKILL.md | 104 ++++ CLAUDE.md | 2 + README.md | 30 ++ index.qmd | 2 + standards.md | 487 ++++++++++++++++++ 5 files changed, 625 insertions(+) create mode 100644 .claude/skills/review-python-package/SKILL.md create mode 100644 standards.md diff --git a/.claude/skills/review-python-package/SKILL.md b/.claude/skills/review-python-package/SKILL.md new file mode 100644 index 0000000..fab5404 --- /dev/null +++ b/.claude/skills/review-python-package/SKILL.md @@ -0,0 +1,104 @@ +--- +name: review-python-package +description: Audit a Python package repository against the DHI Python package standards — layout, pyproject, dependencies, tests, code smells, docs, CI and release setup. Produces a findings report with links to the rule each finding breaks, and offers to open GitHub issues. Use when asked to review, audit or health-check a Python package or repo, or asked "is this a proper package yet". +--- + +# Review a Python package + +Audit a whole repository against the DHI Python package standards. This is a repository +audit — it judges the package as it stands, not a single change. If asked to review a pull +request or a diff, review only the changed files against the same standards. + +## Standards + +The rules live in `standards.md` at the root of the `python-package-development` repository, +published at: + +``` +https://dhi.github.io/python-package-development/standards.html +``` + +Read the local file if this skill is running inside that repository +(`../../../standards.md` from this skill's directory); otherwise fetch the published page. +**Read it before reviewing** — do not audit from memory. Every rule carries a severity +(*Blocker* / *Recommended* / *Nice*) and its heading is the anchor you cite. + +## Gather evidence + +Work from what the repository actually contains. Do not guess at numbers you can measure. + +Run these — fast, read-only, no side effects: + +```bash +ls -a # layout, .gitignore, LICENSE, README +cat pyproject.toml # build system, metadata, dependencies, versioning +ls .github/workflows/ && cat .github/workflows/*.yml +ruff check . # ground truth, not a guess +ruff format --check . +git log --oneline -20 # commit hygiene +git ls-files | grep -Ei '\.(csv|nc|dfs.|xlsx|zip|parquet)$' # data in git +``` + +Then read the source: `src/` (or the package directory), `tests/`, `docs/`. + +**Do not run** `pytest`, `mypy` or `uv sync` on your own initiative — they are slow, may need +network or credentials, and an unfamiliar test suite may have side effects. Report what the +test suite looks like and offer to run it. + +## Check for the code smells the course names + +Grep as a starting point, then read the hits — a grep match is a candidate, not a finding. + +| Rule | Starting point | +| --- | --- | +| Mutable default arguments | `grep -rEn 'def .*=\s*(\[\]\|\{\}\|set\(\))' src/` | +| Class variables that should be instance variables | mutable assignment in a class body, outside `__init__` | +| Modified input arguments | assignment to a parameter's elements inside a function | +| Mixed return types | multiple `return` statements of different types in one function | +| Silently swallowed errors | `grep -rn 'except.*:\s*pass\|except:' src/` | +| Java-like API | `grep -rEn 'def [a-z]+[A-Z]' src/` | +| Reaching into another object's internals | `grep -rEn '\w+\.\w*\._[a-z]' src/` — exclude `self._` | +| Missing docstrings | public functions and classes with no `"""` | +| Removals with no deprecation path | `git log -p` on the public API vs `CHANGELOG.md` | + +## Report + +Print the report in the conversation. Do not write a file unless asked. + +- One-line verdict first — is this a package someone can install and depend on, or not. +- Then **Blockers**, **Recommended**, **Nice** in that order. Omit empty sections. +- Every finding: `file:line` where there is one, what is wrong in one line, the fix, and the + anchor URL of the rule. +- Say what you checked and found clean — a short "✓ Packaging, docs, CI" line. Silence reads + as "not checked". +- Report what you did not check and why (tests not run, package not installed). + +``` +**my_library** — not installable yet + +Blockers + ✗ pyproject.toml — no [build-system], so pip cannot build this + https://dhi.github.io/python-package-development/standards.html#pyproject.toml + ✗ No LICENSE — effectively all rights reserved, colleagues cannot legally use it + https://dhi.github.io/python-package-development/standards.html#license + ⚠ src/clean.py:22 — mutable default `cart=[]` is shared across every call; use None + https://dhi.github.io/python-package-development/standards.html#mutable-default-arguments + +Recommended + ⚠ No .github/workflows/ — tests never run outside your machine + https://dhi.github.io/python-package-development/standards.html#ci-on-every-push-and-pull-request + +✓ Clean: layout, README, naming, dependency groups +Not checked: test suite not run (offer: `uv run pytest`) +``` + +Do not pad the report. A package that is in good shape gets a short report saying so. + +## Afterwards + +Offer, do not act: + +- Open a GitHub issue per blocker (`gh issue create`) — **ask first, and confirm the target + repository**. Creating issues is outward-facing. +- Run the test suite, or `mypy`. +- Fix the findings. diff --git a/CLAUDE.md b/CLAUDE.md index 6508183..1091a59 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -16,6 +16,8 @@ Educational course repository for "Python package development 2025" by DHI. Cour ## Repository Structure +- `standards.md` — The course distilled into linkable rules; published at `standards.html` and used as the rubric by the `review-python-package` skill. Keep the two in sync. +- `.claude/skills/review-python-package/` — Skill that audits a Python package against `standards.md` - `*.qmd` files — Course modules (00-07), each a Quarto slide deck - `_quarto.yml` — Quarto site configuration - `projects/data_cleaning/` — Capstone homework project (progressive weekly assignments) diff --git a/README.md b/README.md index fe02946..de9679d 100644 --- a/README.md +++ b/README.md @@ -2,6 +2,34 @@ This repo contains slides used in the course "Python package development". +## Python package standards + +The whole course distilled into one page of rules — every rule has its own link, so you can +point someone at exactly the thing you mean. + +**** + +``` +…/standards.html#mutable-default-arguments +…/standards.html#composition-over-inheritance +…/standards.html#libraries-loose-applications-pinned +``` + +Source: [`standards.md`](standards.md). + +### Reviewing a package against the standards + +[`.claude/skills/review-python-package/`](.claude/skills/review-python-package/) is a +[Claude Code](https://claude.com/claude-code) skill that audits a package repository against +these standards — layout, `pyproject.toml`, dependencies, tests, code smells, docs, CI and +release setup — and reports findings linked to the rule each one breaks. + +Copy the folder into `~/.claude/skills/` (or your project's `.claude/skills/`), then: + +``` +> review this package against the DHI standards +``` + ## Course description Python is the language of choice for data science, scientific computing and AI. @@ -47,6 +75,8 @@ CI/CD via GitHub Actions (`.github/workflows/publish.yml`) renders and publishes ## Repository Structure +- `standards.md` — The course distilled into linkable rules (published as `standards.html`) +- `.claude/skills/review-python-package/` — Claude Code skill that audits a package against `standards.md` - `*.qmd` — Course modules (00-07), each a Quarto reveal.js slide deck - `_quarto.yml` — Quarto site configuration - `projects/data_cleaning/` — Capstone homework project (progressive weekly assignments) diff --git a/index.qmd b/index.qmd index 2471520..957cea0 100644 --- a/index.qmd +++ b/index.qmd @@ -2,6 +2,8 @@ title: "Python package development" --- +[**Python package standards**](standards.md) — the rules from the course, on one page. + [Introduction](00_introduction.qmd) ## Learning modules diff --git a/standards.md b/standards.md new file mode 100644 index 0000000..fe80a74 --- /dev/null +++ b/standards.md @@ -0,0 +1,487 @@ +# Python package standards + +The rules from the [Python package development](index.qmd) course, distilled into one page. +Every section is a rule you can link to directly. + +**Blocker** — the package is not fit to share until this is fixed. +**Recommended** — expected of a DHI package. +**Nice** — worth doing when you get to it. + +## Repository + +### Small, focused pull requests +*Recommended.* One concern per pull request. Commit often, with messages that say what +changed and why. Track work with issues. + +### No data in git +*Blocker.* Only very small test fixtures belong in the repository. Use `.gitignore` for +everything generated. + +### No credentials in git +*Blocker.* Passwords, tokens and connection strings go in GitHub secrets or a secret store — +never in the repository, not even in history. + +## Layout + +### src layout +*Recommended.* Package code under `src/my_library/`, tests in `tests/`, docs in `docs/`. +Importing then tests the *installed* package, not the working directory. + +``` +my_library/ +├── .github/workflows/ +├── src/my_library/__init__.py +├── tests/test_*.py +├── docs/index.md +├── pyproject.toml +├── uv.lock +├── README.md +└── LICENSE +``` + +### Modules group related code +*Recommended.* A module is one `.py` file; a package is a directory of modules with +`__init__.py`. Split by what the code is about, not by file size. + +### Explicit public API +*Recommended.* `__init__.py` re-exports the names users should touch; internal modules are +named with a leading underscore. What you export is what you have to keep working. + +```python +from ._pfsdocument import PfsDocument # mikeio.PfsDocument is the supported name +``` + +### Underscore means internal +*Recommended.* A leading underscore says "not part of the public API". You may change or +remove `_foo` without it counting as a [breaking change](#breaking-changes-bump-major) — +anyone importing it did so at their own risk. Declare the public surface with `__all__` so +the boundary is explicit rather than implied. (Double underscore, `__foo`, is name mangling — +a different thing.) + +### Naming conventions +*Recommended.* `lowercase_with_underscores` for variables, functions and methods; +`CamelCase` for classes; `UPPERCASE_WITH_UNDERSCORES` for constants. + +## Packaging + +### pyproject.toml +*Blocker.* A package without `[build-system]` and `[project]` is not installable. `uv init +--lib` gives you a working one. + +```toml +[build-system] +requires = ["uv_build>=0.8.9,<0.9.0"] +build-backend = "uv_build" + +[project] +name = "my_library" +version = "0.0.1" +description = "Useful library" +readme = "README.md" +requires-python = ">=3.12" +authors = [{ name="First Last", email="initials@dhigroup.com" }] +dependencies = ["numpy"] + +[project.urls] +"Homepage" = "https://github.com/DHI/my_library" +"Bug Tracker" = "https://github.com/DHI/my_library/issues" +``` + +### Semantic versioning +*Recommended.* `{major}.{minor}.{patch}` — major means breaking, minor means new features, +patch means fixes. Start at `0.1.0`. `1.0` is a promise that the API is stable. + +### Breaking changes bump major +*Blocker.* Removing a function, renaming one, or changing a signature — including reordering +positional arguments — breaks callers. Avoid it; when you can't, bump the major version. This +applies to the [public API](#underscore-means-internal) only. + +### Deprecate before removing +*Recommended.* Warn in one version, remove in the next major — never both at once. Give +people at least a release to migrate, and say in the message what to use instead. + +```python +from warnings import deprecated # Python 3.13+ + +@deprecated("Use new_function instead; removed in 2.0") +def old_function(x): ... +``` + +`DeprecationWarning` is for developers (hidden by default, shows in test runs); +`FutureWarning` is for end users (always visible). `mypy --enable-error-code=deprecated` +catches uses of `@deprecated` at type-check time. + +### Changelog +*Recommended.* A `CHANGELOG.md` in [keepachangelog](https://keepachangelog.com/) format. +Release notes written from a git log are not release notes — the reader wants to know what +broke, what's new, and what's deprecated. + +### License +*Blocker.* Without a license the package is "all rights reserved" and legally unusable by +others. MIT for open, a copyright notice for internal-only. Check your dependencies' licenses +too. + +``` +# Copyright (c) DHI +# All rights reserved. +``` + +## Dependencies + +### Every dependency is a decision +*Recommended.* You are shipping someone else's code to your users, and pulling in everything +*it* depends on. Before adding one, check: is it maintained, what's the license (GPL can force +your package to be GPL), and does it need compiled extensions that will break installation on +a colleague's laptop? `uv pip tree` shows what you actually ship. + +Neither extreme is right — don't reinvent NumPy, but don't take a dependency for twenty lines +you could write and understand yourself. + +### Libraries loose, applications pinned +*Recommended.* A library is imported by other code, so keep bounds wide (`numpy>=1.11.0`) to +avoid conflicting with whatever else the user has installed. An application is run by a user, +so pin (`numpy==1.11.0`) for reproducibility. + +### Development dependencies are separate +*Recommended.* pytest, ruff, mypy and mkdocs are needed to *develop* the package, not to +*run* it. They belong in `[dependency-groups]`, not `[project].dependencies`. + +```toml +[dependency-groups] +dev = ["pytest", "ruff", "mypy", "mkdocs", "mkdocstrings[python]", "mkdocs-material"] +``` + +### uv for environments and locking +*Recommended.* One virtual environment per project, managed by `uv`. Commit `uv.lock` so +everyone resolves to the same set of packages. + +```bash +uv add matplotlib uv add --dev pytest +uv sync uv run pytest +``` + +## Testing + +### Tests exist and are automated +*Blocker.* `pytest`, in `tests/`, runnable with one command. Manual checking does not survive +the next change. + +### Good unit tests +*Recommended.* Fast, in-memory, deterministic, order-independent, and each one about a single +logical concept. No database, no network, no random numbers. + +### Test the edges +*Recommended.* Empty list, single element, empty string, empty dict, `None`, `np.nan`. That is +where the bugs are. + +### Tests document behaviour +*Recommended.* A test name should state a rule. Someone reading the test file should learn how +the code is meant to behave. + +```python +def test_operable_period_can_be_missing(): + assert is_operable(height=1.0, period=None) + +def test_height_can_not_be_missing(): + with pytest.raises(ValueError): + is_operable(height=None) +``` + +### Meaningful coverage +*Nice.* `pytest --cov=my_library` to find untested code. Use the report to aim tests, not to +chase a number. + +## Code + +### Mutable default arguments +*Blocker.* Defaults are evaluated once, when the function is defined — not per call. A mutable +default is shared by every call, forever. + +```python +def add_to_cart(x, cart=[]): # one shared list +def add_to_cart(x, cart=None): # ✓ then: if cart is None: cart = [] +``` + +### Don't modify input arguments +*Recommended.* Arguments are passed by reference, so mutating them surprises the caller. +Return a new object instead. + +```python +def clip(values): + for i in range(len(values)): # caller's list silently changed + values[i] = min(0, values[i]) + +def clip(values): + return [min(0, v) for v in values] # ✓ +``` + +### One return type +*Blocker.* A function that returns a `bool` on success and a `str` on failure will read as +success — a non-empty string is truthy. + +```python +if is_operable(height=12.0, period=5.0): # returns "No way!" — and this runs + print("Go ahead!") +``` + +### Errors should never pass silently +*Blocker.* Raise rather than let a bad value propagate. Exceptions are how your code talks to +its user. Use built-ins (`ValueError`, `KeyError`, `FileNotFoundError`) or define your own +where the domain warrants it. Never swallow with a bare `except`. + +```python +if height < 0.0: + raise ValueError(f"Supplied value of {height=} is unphysical.") +``` + +### Pure functions where you can +*Recommended.* Same input, same output, no side effects — easier to reason about and trivial +to test. Where side effects are necessary (files, databases, plots), keep them deliberate and +in few places. + +### Instance variables, not class variables +*Blocker.* A list defined in the class body is shared by every instance. Assign in `__init__`. + +```python +class Toolbox: + tools = [] # shared by all instances + def __init__(self): + self.tools = [] # ✓ one per object +``` + +### Type hints +*Recommended.* On public functions at minimum. They are hints, not enforcement — they exist +for the reader and the editor. + +```python +def clip(values: list[int], *, threshold: int = 0) -> list[int]: ... +``` + +### Keyword-only arguments +*Nice.* `def f(*, x, y)` forces callers to be explicit and lets you reorder parameters later +without breaking anyone. + +### Dataclasses for data +*Recommended.* Fields with type hints, a constructor, a useful `repr`, and equality by value +rather than by identity — for free. + +```python +@dataclass +class Interval: + start: date + end: date +``` + +### Composed methods +*Recommended.* Each function does one identifiable task, and all operations inside it sit at +the same level of abstraction. Expect many small functions. A script split by comments is +asking to be split into functions. + +```python +def main(): + df = get_data("raw_data.csv") + cleaned = clean_data(df) + final = transform_data(cleaned) + return predict(final) +``` + +### Comments say why, not what +*Recommended.* A comment that restates the code is noise that goes stale. Write the ones that +capture what the code cannot say — the reason. If you need a comment to explain *what* is +happening, rename something instead. + +```python +# Calculate the average temperature ← says nothing the code doesn't +# Sensors report -999 when disconnected ← you could not have known this +``` + +### When a long signature is a smell +*Nice.* Many optional keyword arguments with sane defaults are perfectly Pythonic — see +`read_csv`, `plot`, or any sklearn estimator. The smell is not the count; it's when the +arguments are switches for **separate jobs** the function has absorbed. If half the signature +only applies when another argument is set, that's several functions wearing one signature. + +Then: group related parameters into a config dataclass, offer named presets, or split into +composable pieces that each do one thing. + +```python +plot_scatter(ax, x, y, show_density=True) # ✓ each does one job +plot_reg_line(ax, x, y) +add_skill_table(ax, x, y, metrics=["bias"]) +``` + +### Names carry meaning +*Recommended.* `n_freezing_days` over `n`, `FREEZING_POINT` over `0.0`. Renaming is the +cheapest refactoring there is. + +## Design + +### Composition over inheritance +*Recommended.* Composition is "has a", inheritance is "is a". Use inheritance only to +specialize behaviour — most of the time composition is the better fit. + +### Encapsulate invariants +*Recommended.* A rule enforced only in `__init__` does not survive assignment. Use `_name` +plus a property when the invariant must hold. + +```python +@property +def name(self): return self._name + +@name.setter +def name(self, value): self._name = value.upper() +``` + +### Don't reach into other classes +*Blocker.* Classes talk through public APIs. Touching another object's `_private` attributes +couples you to its internals, and it will break. If you need something that isn't public, the +other class is missing a method — add it there. + +```python +values = values[self.da.geometry.top_elements] # reaching in +da = da.sel(layers="top") # ✓ ask it properly +``` + +### Pythonic over Java-like +*Recommended.* Implement the dunder and get the language feature: `__len__` for `len(obj)`, +`__contains__` for `in`, `__iter__` for `for`, `__getitem__` for `obj[key]`. Your objects +should feel like the built-in types — `tb["hammer"]`, not `tb.getToolByName("hammer")`. + +### Duck typing +*Recommended.* The caller cares that the methods exist, not what the type is. No base class or +interface required — that is what makes a scikit-learn transformer work. + +### Postel's law +*Recommended.* Be liberal in what you accept, conservative in what you send. Normalize input +types once, at the boundary. Pydantic does this for you. + +```python +def process(number: int | str | float) -> int: + number = int(number) + return number * 2 +``` + +### Right level of abstraction +*Nice.* Too little means boilerplate everywhere; too much means nothing can be adapted. +`sum(values)` over a loop, but not a framework where a function would do. + +## Documentation + +### README +*Blocker.* What the package does, what it requires (OS, Python version, non-Python +dependencies), and how to install it. + +```bash +pip install my_library +pip install https://github.com/DHI/my_library/archive/main.zip +``` + +### Docstrings, numpy format +*Recommended.* On every public function and class. Written once, read in `help()`, in the +editor tooltip, and on the generated API site. Numpy format is the DHI default — set +`docstring_style: "numpy"` in mkdocs, since the default is google. + +```python +def remove_outlier(data: pd.DataFrame, column: str, threshold: float = 3) -> pd.DataFrame: + """Remove outliers from a dataframe. + + Parameters + ---------- + threshold : float, optional + Number of standard deviations to use as threshold, by default 3 + + Returns + ------- + pd.DataFrame + Dataframe with outliers removed. + """ +``` + +### Examples that are tested +*Nice.* `doctest` runs the examples in your docstrings. Documentation that is wrong is worse +than documentation that is missing. + +```bash +python -m doctest -v add.py +``` + +### Published API documentation +*Recommended.* `mkdocs` + `mkdocstrings` + GitHub Pages, at +`https://dhi.github.io//`. Note that a private repository can still have a public +website — `robots.txt` hides it from search engines but is not security. + +## Automation + +### CI on every push and pull request +*Blocker.* A workflow in `.github/workflows/` that installs and runs the tests. This is what +solves "it works on my machine". + +```yaml +on: + push: { branches: [main] } + pull_request: { branches: [main] } + +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - uses: astral-sh/setup-uv@v6 + with: { python-version: "3.13" } + - run: uv sync + - run: uv run pytest +``` + +### Lint and format with ruff +*Recommended.* `ruff check` finds unused imports, undefined names and dead variables — usually +typos, sometimes bugs. `ruff format` ends style arguments. Run both in CI. + +```bash +ruff check . ruff format --check . +``` + +### Makefile +*Nice.* One source of truth for how to run the project's tools, and the fastest onboarding +document there is. + +```makefile +check: lint test +lint: + ruff check src +test: + pytest +``` + +### Test the matrix +*Nice.* If you claim to support Windows and Python 3.10, test on Windows and Python 3.10. + +```yaml +strategy: + matrix: + os: [ubuntu-latest, windows-latest] + python-version: ["3.10", "3.13"] +``` + +## Release + +### Publish from a GitHub release +*Recommended.* Tag a release, let a workflow build and publish. Use Trusted Publishers so +there are no secrets to manage. + +```yaml +on: + release: + types: [published] +``` + +### Somewhere to install from +*Recommended.* PyPI for public packages; Azure Artifacts or Posit Package Manager for internal +ones. Straight from GitHub works too, and needs no index at all. + +```bash +pip install https://github.com/DHI/mikeio/archive/main.zip +``` + +### Pre-releases for anything unfinished +*Nice.* `1.0.0rc1` is not installed by default and does not appear in search — the safe way to +put something in front of users before committing to it. From 0055f5398d0d8fe3ee2ad4f8151cc8cd4d20bc96 Mon Sep 17 00:00:00 2001 From: Henrik Andersson Date: Thu, 6 Aug 2026 20:11:12 +0200 Subject: [PATCH 2/5] Address review comments on standards and review skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .claude/skills/review-python-package/SKILL.md | 58 +++++--- standards.md | 124 ++++++++++++++---- 2 files changed, 138 insertions(+), 44 deletions(-) diff --git a/.claude/skills/review-python-package/SKILL.md b/.claude/skills/review-python-package/SKILL.md index fab5404..1fe00a0 100644 --- a/.claude/skills/review-python-package/SKILL.md +++ b/.claude/skills/review-python-package/SKILL.md @@ -27,37 +27,61 @@ Read the local file if this skill is running inside that repository Work from what the repository actually contains. Do not guess at numbers you can measure. -Run these — fast, read-only, no side effects: +The reviewer may be on Windows without a POSIX shell, so use the Glob, Read and Grep tools +for anything that inspects files — never `ls`, `cat`, or a pipe into `grep`. + +| What | How | +| --- | --- | +| Layout, `.gitignore`, `LICENSE`, `README` | Glob `*` and `.*` at the root | +| Build system, metadata, dependencies, versioning | Read `pyproject.toml` | +| CI | Glob `.github/workflows/*`, then Read each | +| Data committed to git | `git ls-files "*.csv" "*.nc" "*.dfs*" "*.xlsx" "*.zip" "*.parquet"` | +| Commit hygiene | `git log --oneline -20` | + +Then run the linter — ground truth, not a guess. `uvx` so it works without the repository's +environment installed: ```bash -ls -a # layout, .gitignore, LICENSE, README -cat pyproject.toml # build system, metadata, dependencies, versioning -ls .github/workflows/ && cat .github/workflows/*.yml -ruff check . # ground truth, not a guess -ruff format --check . -git log --oneline -20 # commit hygiene -git ls-files | grep -Ei '\.(csv|nc|dfs.|xlsx|zip|parquet)$' # data in git +uvx ruff check . +uvx ruff format --check . ``` +`ruff` reads the repository's `[tool.ruff]` config if there is one. If there is none you are +seeing the default ruleset, which is not the same as the project's intent — say so in the +report rather than presenting the output as the project's own standard. + Then read the source: `src/` (or the package directory), `tests/`, `docs/`. **Do not run** `pytest`, `mypy` or `uv sync` on your own initiative — they are slow, may need network or credentials, and an unfamiliar test suite may have side effects. Report what the -test suite looks like and offer to run it. +test suite looks like and offer to run it. Whether a type checker runs at all is visible in +the workflow file — judge that from reading it, not by running one. ## Check for the code smells the course names -Grep as a starting point, then read the hits — a grep match is a candidate, not a finding. +Search as a starting point, then read the hits — a match is a candidate, not a finding. + +Use the Grep tool over `**/*.py`, showing line numbers. These are regular expressions for +that tool, not shell commands — do not wrap them in `grep`, and do not escape the `|`. + +``` +def .*=\s*(\[\]|\{\}|set\(\)|dict\(\)|list\(\)) # mutable default arguments +except\s*: # bare except +except.*:\s*pass # swallowed error, one-line form +def [a-z]+[A-Z] # java-like API +\w+\.\w*\._[a-z] # reaching into another object's internals +``` + +`except.*:\s*pass` only catches the one-line form, so read every `except` block you find. For +the last pattern, discard the `self._` hits — those are the class's own internals. + +The rest have no pattern worth writing; read for them: -| Rule | Starting point | +| Rule | What to look for | | --- | --- | -| Mutable default arguments | `grep -rEn 'def .*=\s*(\[\]\|\{\}\|set\(\))' src/` | | Class variables that should be instance variables | mutable assignment in a class body, outside `__init__` | | Modified input arguments | assignment to a parameter's elements inside a function | | Mixed return types | multiple `return` statements of different types in one function | -| Silently swallowed errors | `grep -rn 'except.*:\s*pass\|except:' src/` | -| Java-like API | `grep -rEn 'def [a-z]+[A-Z]' src/` | -| Reaching into another object's internals | `grep -rEn '\w+\.\w*\._[a-z]' src/` — exclude `self._` | | Missing docstrings | public functions and classes with no `"""` | | Removals with no deprecation path | `git log -p` on the public API vs `CHANGELOG.md` | @@ -68,7 +92,7 @@ Print the report in the conversation. Do not write a file unless asked. - One-line verdict first — is this a package someone can install and depend on, or not. - Then **Blockers**, **Recommended**, **Nice** in that order. Omit empty sections. - Every finding: `file:line` where there is one, what is wrong in one line, the fix, and the - anchor URL of the rule. + anchor URL of the rule. `✗` for Blockers, `⚠` for everything else. - Say what you checked and found clean — a short "✓ Packaging, docs, CI" line. Silence reads as "not checked". - Report what you did not check and why (tests not run, package not installed). @@ -81,7 +105,7 @@ Blockers https://dhi.github.io/python-package-development/standards.html#pyproject.toml ✗ No LICENSE — effectively all rights reserved, colleagues cannot legally use it https://dhi.github.io/python-package-development/standards.html#license - ⚠ src/clean.py:22 — mutable default `cart=[]` is shared across every call; use None + ✗ src/clean.py:22 — mutable default `cart=[]` is shared across every call; use None https://dhi.github.io/python-package-development/standards.html#mutable-default-arguments Recommended diff --git a/standards.md b/standards.md index fe80a74..3d0e553 100644 --- a/standards.md +++ b/standards.md @@ -112,9 +112,15 @@ def old_function(x): ... catches uses of `@deprecated` at type-check time. ### Changelog -*Recommended.* A `CHANGELOG.md` in [keepachangelog](https://keepachangelog.com/) format. -Release notes written from a git log are not release notes — the reader wants to know what -broke, what's new, and what's deprecated. +*Nice.* A `CHANGELOG.md` in [keepachangelog](https://keepachangelog.com/) format. Release +notes written from a git log are not release notes — the reader wants to know what broke, +what's new, and what's deprecated. + +Curating one by hand is real work, and a stale changelog is worse than none. Think twice +before starting: if nobody reads it, skip it. If you do want one, let a tool assemble it from +a fragment per pull request — [towncrier](https://towncrier.readthedocs.io/) or +[git-cliff](https://git-cliff.org/) — so the cost lands on the author of each change rather +than on you at release time. ### License *Blocker.* Without a license the package is "all rights reserved" and legally unusable by @@ -209,18 +215,26 @@ Return a new object instead. ```python def clip(values): for i in range(len(values)): # caller's list silently changed - values[i] = min(0, values[i]) + values[i] = max(0, values[i]) def clip(values): - return [min(0, v) for v in values] # ✓ + return [max(0, v) for v in values] # ✓ ``` ### One return type *Blocker.* A function that returns a `bool` on success and a `str` on failure will read as success — a non-empty string is truthy. +A function with a `return` on one path and nothing on another is the same bug: the missing +path returns `None`. + ```python -if is_operable(height=12.0, period=5.0): # returns "No way!" — and this runs +def is_operable(height, period): + if height > 10.0: + return "No way!" # str here, None on every other path + return True # ...and bool here + +if is_operable(height=12.0, period=5.0): # "No way!" is truthy — this runs print("Go ahead!") ``` @@ -251,15 +265,24 @@ class Toolbox: ### Type hints *Recommended.* On public functions at minimum. They are hints, not enforcement — they exist -for the reader and the editor. +for the reader and the editor, until you add a [type checker](#type-checking-in-ci). ```python def clip(values: list[int], *, threshold: int = 0) -> list[int]: ... ``` ### Keyword-only arguments -*Nice.* `def f(*, x, y)` forces callers to be explicit and lets you reorder parameters later -without breaking anyone. +*Recommended.* One or two positional parameters is fine — that is the data the function +operates on. Everything after them is configuration, and belongs after a `*` so callers have +to name it. You can then reorder or add options without breaking anyone. + +Three or more positional parameters is a strong smell: `resample(df, 3, 0, True)` can't be +read at the call site, and nobody can safely change the order again. + +```python +def resample(data, freq, *, offset=0, dropna=True): ... +resample(df, "1h", dropna=False) # ✓ the data and its frequency; the rest is named +``` ### Dataclasses for data *Recommended.* Fields with type hints, a constructor, a useful `repr`, and equality by value @@ -297,7 +320,8 @@ happening, rename something instead. ### When a long signature is a smell *Nice.* Many optional keyword arguments with sane defaults are perfectly Pythonic — see -`read_csv`, `plot`, or any sklearn estimator. The smell is not the count; it's when the +`read_csv`, `plot`, or any sklearn estimator. The smell is not the total count (that's +[positional arguments](#keyword-only-arguments), which are a separate rule); it's when the arguments are switches for **separate jobs** the function has absorbed. If half the signature only applies when another argument is set, that's several functions wearing one signature. @@ -398,8 +422,9 @@ def remove_outlier(data: pd.DataFrame, column: str, threshold: float = 3) -> pd. ``` ### Examples that are tested -*Nice.* `doctest` runs the examples in your docstrings. Documentation that is wrong is worse -than documentation that is missing. +*Nice.* Documentation that is wrong is worse than documentation that is missing. `doctest` +runs the examples in your docstrings. For prose pages, [Quarto](https://quarto.org/) executes +every snippet as part of the build, so the docs cannot ship broken — the build fails first. ```bash python -m doctest -v add.py @@ -407,8 +432,12 @@ python -m doctest -v add.py ### Published API documentation *Recommended.* `mkdocs` + `mkdocstrings` + GitHub Pages, at -`https://dhi.github.io//`. Note that a private repository can still have a public -website — `robots.txt` hides it from search engines but is not security. +`https://dhi.github.io//`. [Quarto](https://quarto.org/), +[Great Docs](https://github.com/machow/great-docs) (which wraps Quarto) and +[zensical](https://zensical.org/) are viable alternatives. + +A private repository can have access-controlled Pages on GitHub Enterprise — use that when the +site should stay internal, rather than relying on the URL not being found. ## Automation @@ -425,7 +454,7 @@ jobs: test: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - uses: astral-sh/setup-uv@v6 with: { python-version: "3.13" } - run: uv sync @@ -433,28 +462,55 @@ jobs: ``` ### Lint and format with ruff -*Recommended.* `ruff check` finds unused imports, undefined names and dead variables — usually -typos, sometimes bugs. `ruff format` ends style arguments. Run both in CI. +*Recommended.* There is no reason not to. One binary, no configuration required, and it +replaces flake8, black and isort at once. `ruff check` finds unused imports, undefined names +and dead variables — usually typos, sometimes bugs. `ruff format` ends style arguments. Run +both in CI. ```bash ruff check . ruff format --check . ``` -### Makefile +### Type checking in CI +*Nice.* [Type hints](#type-hints) are not enforcement — a type checker is. Run `mypy` (or +`ty`) in CI on the package, not the tests, and turn it on for new code before old. It also +catches uses of anything you have marked +[`@deprecated`](#deprecate-before-removing). + +```bash +uv run mypy src --enable-error-code=deprecated +``` + +### A task runner *Nice.* One source of truth for how to run the project's tools, and the fastest onboarding -document there is. +document there is. A `Makefile` if everyone is on Linux or macOS; `just` if anyone is on +Windows, where `make` is not installed by default and `just` is a single binary +(`uv tool install rust-just`). ```makefile -check: lint test +check: lint test # Makefile +lint: + uv run ruff check src +test: + uv run pytest +``` + +```just +check: lint test # justfile lint: - ruff check src + uv run ruff check src test: - pytest + uv run pytest ``` ### Test the matrix *Nice.* If you claim to support Windows and Python 3.10, test on Windows and Python 3.10. +Test what you claim and no more. CI is not free — every cell costs minutes on every push. An +[application](#libraries-loose-applications-pinned) has one deployment target, so one cell is +the honest matrix; a library that others install needs the range it advertises in +`requires-python`. + ```yaml strategy: matrix: @@ -464,14 +520,28 @@ strategy: ## Release -### Publish from a GitHub release -*Recommended.* Tag a release, let a workflow build and publish. Use Trusted Publishers so -there are no secrets to manage. +### Tag every release +*Recommended.* An annotated `vX.Y.Z` tag, pushed. It is what makes "which commit is 1.2.0?" +answerable a year later, and what lets you diff two releases. Just do it — it costs one +command. + +```bash +git tag -a v1.2.0 -m "v1.2.0" +git push --tags +``` + +### Publish from a tag or a release +*Recommended.* Let a workflow build and publish; never upload from your laptop. Use +[Trusted Publishers](https://docs.pypi.org/trusted-publishers/) so there are no secrets to +manage. + +Trigger on the tag, or on a published GitHub release — either works with Trusted Publishers. +The release gives you somewhere to put release notes; the tag is one step fewer. ```yaml on: - release: - types: [published] + push: + tags: ["v*"] # or: release: { types: [published] } ``` ### Somewhere to install from From 0a1f947b584629e18b318e320ca056bd7ddd7b0f Mon Sep 17 00:00:00 2001 From: Henrik Andersson Date: Tue, 25 Aug 2026 15:49:17 +0200 Subject: [PATCH 3/5] Add standards rule: tests must run from a clean clone --- standards.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/standards.md b/standards.md index 3d0e553..0406b0a 100644 --- a/standards.md +++ b/standards.md @@ -172,6 +172,33 @@ uv sync uv run pytest *Blocker.* `pytest`, in `tests/`, runnable with one command. Manual checking does not survive the next change. +### Tests run from a clean clone +*Blocker.* Resolve test data relative to the test file. Never an absolute or home-relative +path, and **never as a fallback default** — a default like +`os.environ.get("REF_DATA", Path.home() / "data")` looks configurable but only ever resolves +on its author's machine. + +```python +TESTDATA = Path(__file__).parent / "testdata" # good +TESTDATA = Path.home() / "src" / "ref" / "TestData" # never +``` + +The wrong path is only the symptom. The damage is that the test passes for its author and +skips for everyone else, including CI, so the suite reports green while verifying nothing. + +A skip is not a fix. This was a common accident before CI was the norm; now the more likely +version is a `skip` added deliberately — often by a coding agent — to get a red suite green. +That is worse than the failure it hides, because it turns a visible problem into an invisible +one. A skip whose condition is false only on your machine is a hole with no bottom. + +Make CI fail on unexpected skips rather than trusting the summary line. pytest has no +built-in flag for this, but a small `conftest.py` hook that turns a skip into a failure does +the job. Then read the skip list in review: every remaining skip should have a reason you +would defend out loud. + +If the data cannot be committed (see *No data in git*), the check belongs in a script, not in +the test suite. + ### Good unit tests *Recommended.* Fast, in-memory, deterministic, order-independent, and each one about a single logical concept. No database, no network, no random numbers. From a7e36f359dd22c3bf351f1955a2d343587080b3a Mon Sep 17 00:00:00 2001 From: Henrik Andersson Date: Tue, 25 Aug 2026 15:49:25 +0200 Subject: [PATCH 4/5] Teach just as the task runner The DHI python template migrated from a Makefile to a justfile, so the course now leads with just: cross-platform single binary, self-documenting via just --list. make is kept as a footnote. --- 03_testing.qmd | 32 ++++++++++++--------- projects/data_cleaning/04_Project_module.md | 2 +- standards.md | 18 ++++-------- 3 files changed, 25 insertions(+), 27 deletions(-) diff --git a/03_testing.qmd b/03_testing.qmd index 5270277..1b9ee49 100644 --- a/03_testing.qmd +++ b/03_testing.qmd @@ -502,33 +502,37 @@ Line # Hits Time Per Hit % Time Line Contents 19 1 500.0 500.0 0.0 return points ``` -## Makefiles {.smaller} +## Task runners {.smaller} ::: {.incremental} -* Makefiles simplify running complex commands. -* They act as a single source of truth for how to run your tools. -* Self documenting, making it easier to onboard new team members. -* Run 'make <command>' to run a command from the Makefile. -* On Linux by default, for Windows install with [MSYS2](https://www.msys2.org/) or [Chocolatey](https://chocolatey.org/) (or use WSL). +* A task runner simplifies running complex commands. +* It acts as a single source of truth for how to run your tools. +* Self documenting, making it easier to onboard new team members — and for coding agents to find the right command. +* We use [`just`](https://just.systems): one cross-platform binary, install with `uv tool install rust-just`. +* Run `just `, and `just --list` to see what is available. +* `make` does the same job, but is not installed on Windows and is fussy about tabs. ::: . . . -Example +From the [DHI python template](https://github.com/DHI/template-python-library) -```{.makefile filename="Makefile" code-line-numbers="|1|5-6|8-9|11-12|3|"} -LIB = my_library +```{.just filename="justfile" code-line-numbers="|1|3|5-6|8-9|11-12|14-15|"} +LIB := "my_library" -check: lint typecheck test +check: lint typecheck test doctest lint: - ruff check $(LIB) + uv run ruff check {{LIB}} format: - ruff format $(LIB) + uv run ruff format {{LIB}} test: - pytest --disable-warnings + uv run pytest --disable-warnings + +typecheck: + uv run mypy {{LIB}}/ --config-file pyproject.toml ``` ## Summary {.smaller} @@ -544,6 +548,6 @@ test: * **Ruff** is a fast linter that checks for common errors and style issues. * **Ruff** is also an automatic code formatter that enforces a consistent style. * Profiling is a way to measure the performance of your code. -* **Makefiles** simplify project related commands for everyone. +* **Task runners** like **just** simplify project related commands for everyone. ::: diff --git a/projects/data_cleaning/04_Project_module.md b/projects/data_cleaning/04_Project_module.md index 33754b6..74b5e55 100644 --- a/projects/data_cleaning/04_Project_module.md +++ b/projects/data_cleaning/04_Project_module.md @@ -8,7 +8,7 @@ In this module, we will use some files from the python library template. When yo - Create new branch `action-formatting` (Make sure changes from last module have been merged, and that you start from the main branch) - 4.1 Github Action - - Copy the `Makefile` from the python template to your own library. It can sit in the root of your repo (make sure it is part of your github repo dir however) + - Copy the `justfile` from the python template to your own library (install `just` with `uv tool install rust-just`). It can sit in the root of your repo (make sure it is part of your github repo dir however) - Copy the GitHub action file `full_test.yml` (in the `.github/workflows` folder) from the python template to your own library. Make sure it sits in the same folder (`.github/workflows`). - Change all occurrences of "my_library" in the yml file to your package name "tscleaner" - Comment out the line with `ruff-action` with "#" diff --git a/standards.md b/standards.md index 0406b0a..97bb168 100644 --- a/standards.md +++ b/standards.md @@ -510,20 +510,14 @@ uv run mypy src --enable-error-code=deprecated ### A task runner *Nice.* One source of truth for how to run the project's tools, and the fastest onboarding -document there is. A `Makefile` if everyone is on Linux or macOS; `just` if anyone is on -Windows, where `make` is not installed by default and `just` is a single binary -(`uv tool install rust-just`). - -```makefile -check: lint test # Makefile -lint: - uv run ruff check src -test: - uv run pytest -``` +document there is — for people and for coding agents. Prefer +[`just`](https://just.systems) (`uv tool install rust-just`): a single cross-platform binary, +`Makefile`-like syntax, and `just --list` documents itself. `make` works too, but it is not +installed on Windows and is a build tool pressed into service as a task runner. The +[DHI template](https://github.com/DHI/template-python-library) ships a `justfile`. ```just -check: lint test # justfile +check: lint typecheck test # justfile lint: uv run ruff check src test: From 63e17e32f4eac73814ced7506ac911bb0f0a13c3 Mon Sep 17 00:00:00 2001 From: Henrik Andersson Date: Tue, 25 Aug 2026 16:04:20 +0200 Subject: [PATCH 5/5] Tighten standards to normative one-liners - Cut exposition from the eleven multi-paragraph rules; every rule is now the rule plus its reason, no essays. All 45 anchors unchanged. - Fix the One return type example: the comment claimed a None path the code does not have. - Align requires-python (>=3.11) with the DHI template and with the CI matrix example, which previously contradicted it. - justfile excerpt in 03_testing was not runnable: check depended on a doctest recipe that was elided, and dropped the windows-shell line the cross-platform claim rests on. --- 03_testing.qmd | 8 +- standards.md | 299 +++++++++++++++++++++---------------------------- 2 files changed, 137 insertions(+), 170 deletions(-) diff --git a/03_testing.qmd b/03_testing.qmd index 1b9ee49..e2ea621 100644 --- a/03_testing.qmd +++ b/03_testing.qmd @@ -510,6 +510,7 @@ Line # Hits Time Per Hit % Time Line Contents * Self documenting, making it easier to onboard new team members — and for coding agents to find the right command. * We use [`just`](https://just.systems): one cross-platform binary, install with `uv tool install rust-just`. * Run `just `, and `just --list` to see what is available. +* On Windows, `set windows-shell` makes the recipes run in PowerShell. * `make` does the same job, but is not installed on Windows and is fussy about tabs. ::: @@ -517,7 +518,9 @@ Line # Hits Time Per Hit % Time Line Contents From the [DHI python template](https://github.com/DHI/template-python-library) -```{.just filename="justfile" code-line-numbers="|1|3|5-6|8-9|11-12|14-15|"} +```{.just filename="justfile" code-line-numbers="|1|3|5|7-14|16-20|"} +set windows-shell := ["powershell.exe", "-NoLogo", "-Command"] + LIB := "my_library" check: lint typecheck test doctest @@ -533,6 +536,9 @@ test: typecheck: uv run mypy {{LIB}}/ --config-file pyproject.toml + +doctest: + uv run pytest --doctest-modules {{LIB}} ``` ## Summary {.smaller} diff --git a/standards.md b/standards.md index 97bb168..0898a1c 100644 --- a/standards.md +++ b/standards.md @@ -10,22 +10,21 @@ Every section is a rule you can link to directly. ## Repository ### Small, focused pull requests -*Recommended.* One concern per pull request. Commit often, with messages that say what -changed and why. Track work with issues. +*Recommended.* One concern per pull request. Commit messages say what changed and why. Track +work with issues. ### No data in git -*Blocker.* Only very small test fixtures belong in the repository. Use `.gitignore` for -everything generated. +*Blocker.* Only small test fixtures. Everything generated goes in `.gitignore`. ### No credentials in git -*Blocker.* Passwords, tokens and connection strings go in GitHub secrets or a secret store — -never in the repository, not even in history. +*Blocker.* Passwords, tokens and connection strings belong in GitHub secrets or a secret +store — never in the repository, not even in history. ## Layout ### src layout *Recommended.* Package code under `src/my_library/`, tests in `tests/`, docs in `docs/`. -Importing then tests the *installed* package, not the working directory. +Imports then resolve to the *installed* package, not the working directory. ``` my_library/ @@ -44,19 +43,18 @@ my_library/ `__init__.py`. Split by what the code is about, not by file size. ### Explicit public API -*Recommended.* `__init__.py` re-exports the names users should touch; internal modules are -named with a leading underscore. What you export is what you have to keep working. +*Recommended.* `__init__.py` re-exports the names users should touch. What you export is what +you have to keep working. ```python from ._pfsdocument import PfsDocument # mikeio.PfsDocument is the supported name ``` ### Underscore means internal -*Recommended.* A leading underscore says "not part of the public API". You may change or -remove `_foo` without it counting as a [breaking change](#breaking-changes-bump-major) — -anyone importing it did so at their own risk. Declare the public surface with `__all__` so -the boundary is explicit rather than implied. (Double underscore, `__foo`, is name mangling — -a different thing.) +*Recommended.* A leading underscore says "not part of the public API": `_foo` may change or +disappear without it counting as a [breaking change](#breaking-changes-bump-major). Declare +the public surface with `__all__` so the boundary is explicit. (`__foo` is name mangling — a +different thing.) ### Naming conventions *Recommended.* `lowercase_with_underscores` for variables, functions and methods; @@ -65,7 +63,7 @@ a different thing.) ## Packaging ### pyproject.toml -*Blocker.* A package without `[build-system]` and `[project]` is not installable. `uv init +*Blocker.* Without `[build-system]` and `[project]` the package is not installable. `uv init --lib` gives you a working one. ```toml @@ -78,7 +76,7 @@ name = "my_library" version = "0.0.1" description = "Useful library" readme = "README.md" -requires-python = ">=3.12" +requires-python = ">=3.11" authors = [{ name="First Last", email="initials@dhigroup.com" }] dependencies = ["numpy"] @@ -88,17 +86,17 @@ dependencies = ["numpy"] ``` ### Semantic versioning -*Recommended.* `{major}.{minor}.{patch}` — major means breaking, minor means new features, -patch means fixes. Start at `0.1.0`. `1.0` is a promise that the API is stable. +*Recommended.* `{major}.{minor}.{patch}` — major breaks, minor adds, patch fixes. Start at +`0.1.0`. `1.0` is a promise that the API is stable. ### Breaking changes bump major *Blocker.* Removing a function, renaming one, or changing a signature — including reordering -positional arguments — breaks callers. Avoid it; when you can't, bump the major version. This -applies to the [public API](#underscore-means-internal) only. +positional arguments — breaks callers. Avoid it; when you can't, bump the major version. +Applies to the [public API](#underscore-means-internal) only. ### Deprecate before removing -*Recommended.* Warn in one version, remove in the next major — never both at once. Give -people at least a release to migrate, and say in the message what to use instead. +*Recommended.* Warn in one version, remove in the next major — never both at once. Say what +to use instead. ```python from warnings import deprecated # Python 3.13+ @@ -107,25 +105,19 @@ from warnings import deprecated # Python 3.13+ def old_function(x): ... ``` -`DeprecationWarning` is for developers (hidden by default, shows in test runs); -`FutureWarning` is for end users (always visible). `mypy --enable-error-code=deprecated` -catches uses of `@deprecated` at type-check time. +`DeprecationWarning` targets developers (hidden by default, shown in test runs); +`FutureWarning` targets end users (always visible). `mypy --enable-error-code=deprecated` +catches uses of `@deprecated`. ### Changelog -*Nice.* A `CHANGELOG.md` in [keepachangelog](https://keepachangelog.com/) format. Release -notes written from a git log are not release notes — the reader wants to know what broke, -what's new, and what's deprecated. - -Curating one by hand is real work, and a stale changelog is worse than none. Think twice -before starting: if nobody reads it, skip it. If you do want one, let a tool assemble it from -a fragment per pull request — [towncrier](https://towncrier.readthedocs.io/) or -[git-cliff](https://git-cliff.org/) — so the cost lands on the author of each change rather -than on you at release time. +*Nice.* A `CHANGELOG.md` in [keepachangelog](https://keepachangelog.com/) format: what broke, +what's new, what's deprecated. A git log is not release notes. A stale one is worse than none, +so assemble it from a fragment per pull request ([towncrier](https://towncrier.readthedocs.io/), +[git-cliff](https://git-cliff.org/)) rather than curating it at release time. ### License -*Blocker.* Without a license the package is "all rights reserved" and legally unusable by -others. MIT for open, a copyright notice for internal-only. Check your dependencies' licenses -too. +*Blocker.* No license means all rights reserved and legally unusable by others. MIT for open, +a copyright notice for internal-only. Check your dependencies' licenses too. ``` # Copyright (c) DHI @@ -135,22 +127,19 @@ too. ## Dependencies ### Every dependency is a decision -*Recommended.* You are shipping someone else's code to your users, and pulling in everything -*it* depends on. Before adding one, check: is it maintained, what's the license (GPL can force -your package to be GPL), and does it need compiled extensions that will break installation on -a colleague's laptop? `uv pip tree` shows what you actually ship. - -Neither extreme is right — don't reinvent NumPy, but don't take a dependency for twenty lines -you could write and understand yourself. +*Recommended.* You ship someone else's code, and everything *it* depends on. Check three +things: maintained, license (GPL can force your package to be GPL), and compiled extensions +that will break installation on a colleague's laptop. `uv pip tree` shows what you ship. Don't +reinvent NumPy; don't take a dependency for twenty lines you could own. ### Libraries loose, applications pinned -*Recommended.* A library is imported by other code, so keep bounds wide (`numpy>=1.11.0`) to -avoid conflicting with whatever else the user has installed. An application is run by a user, -so pin (`numpy==1.11.0`) for reproducibility. +*Recommended.* A library is imported by other code — keep bounds wide (`numpy>=1.11.0`) so it +doesn't conflict with what the user already has. An application is run by a user — pin +(`numpy==1.11.0`) for reproducibility. ### Development dependencies are separate -*Recommended.* pytest, ruff, mypy and mkdocs are needed to *develop* the package, not to -*run* it. They belong in `[dependency-groups]`, not `[project].dependencies`. +*Recommended.* pytest, ruff, mypy and mkdocs are needed to *develop* the package, not to *run* +it. Put them in `[dependency-groups]`, not `[project].dependencies`. ```toml [dependency-groups] @@ -159,7 +148,7 @@ dev = ["pytest", "ruff", "mypy", "mkdocs", "mkdocstrings[python]", "mkdocs-mater ### uv for environments and locking *Recommended.* One virtual environment per project, managed by `uv`. Commit `uv.lock` so -everyone resolves to the same set of packages. +everyone resolves to the same packages. ```bash uv add matplotlib uv add --dev pytest @@ -174,42 +163,32 @@ the next change. ### Tests run from a clean clone *Blocker.* Resolve test data relative to the test file. Never an absolute or home-relative -path, and **never as a fallback default** — a default like -`os.environ.get("REF_DATA", Path.home() / "data")` looks configurable but only ever resolves -on its author's machine. +path, and never as a fallback default. ```python TESTDATA = Path(__file__).parent / "testdata" # good TESTDATA = Path.home() / "src" / "ref" / "TestData" # never ``` -The wrong path is only the symptom. The damage is that the test passes for its author and -skips for everyone else, including CI, so the suite reports green while verifying nothing. - -A skip is not a fix. This was a common accident before CI was the norm; now the more likely -version is a `skip` added deliberately — often by a coding agent — to get a red suite green. -That is worse than the failure it hides, because it turns a visible problem into an invisible -one. A skip whose condition is false only on your machine is a hole with no bottom. +The wrong path is the symptom; the damage is a test that passes for its author and skips for +everyone else, so the suite reports green while verifying nothing. -Make CI fail on unexpected skips rather than trusting the summary line. pytest has no -built-in flag for this, but a small `conftest.py` hook that turns a skip into a failure does -the job. Then read the skip list in review: every remaining skip should have a reason you -would defend out loud. - -If the data cannot be committed (see *No data in git*), the check belongs in a script, not in -the test suite. +A skip is not a fix — `skip`, `xfail` or `importorskip` added to turn a suite green trades a +visible failure for an invisible one. Fail CI on unexpected skips (pytest has no flag; use a +`conftest.py` hook). If the data cannot be committed (see +[No data in git](#no-data-in-git)), the check belongs in a script, not the test suite. ### Good unit tests -*Recommended.* Fast, in-memory, deterministic, order-independent, and each one about a single -logical concept. No database, no network, no random numbers. +*Recommended.* Fast, in-memory, deterministic, order-independent, one logical concept each. No +database, no network, no random numbers. ### Test the edges *Recommended.* Empty list, single element, empty string, empty dict, `None`, `np.nan`. That is where the bugs are. ### Tests document behaviour -*Recommended.* A test name should state a rule. Someone reading the test file should learn how -the code is meant to behave. +*Recommended.* A test name states a rule. Someone reading the test file should learn how the +code is meant to behave. ```python def test_operable_period_can_be_missing(): @@ -221,14 +200,14 @@ def test_height_can_not_be_missing(): ``` ### Meaningful coverage -*Nice.* `pytest --cov=my_library` to find untested code. Use the report to aim tests, not to -chase a number. +*Nice.* `pytest --cov=my_library` to find untested code. Aim tests with the report; don't chase +the number. ## Code ### Mutable default arguments -*Blocker.* Defaults are evaluated once, when the function is defined — not per call. A mutable -default is shared by every call, forever. +*Blocker.* Defaults are evaluated once, at definition — not per call. A mutable default is +shared by every call, forever. ```python def add_to_cart(x, cart=[]): # one shared list @@ -236,8 +215,8 @@ def add_to_cart(x, cart=None): # ✓ then: if cart is None: cart = [] ``` ### Don't modify input arguments -*Recommended.* Arguments are passed by reference, so mutating them surprises the caller. -Return a new object instead. +*Recommended.* Arguments are passed by reference, so mutating them surprises the caller. Return +a new object. ```python def clip(values): @@ -249,26 +228,24 @@ def clip(values): ``` ### One return type -*Blocker.* A function that returns a `bool` on success and a `str` on failure will read as -success — a non-empty string is truthy. - -A function with a `return` on one path and nothing on another is the same bug: the missing -path returns `None`. +*Blocker.* One type out, on every path. A function returning `bool` on success and `str` on +failure reads as success — a non-empty string is truthy. A `return` on one path and none on +another is the same bug: the silent path returns `None`. ```python def is_operable(height, period): if height > 10.0: - return "No way!" # str here, None on every other path - return True # ...and bool here + return "No way!" # str here... + return True # ...bool here if is_operable(height=12.0, period=5.0): # "No way!" is truthy — this runs print("Go ahead!") ``` ### Errors should never pass silently -*Blocker.* Raise rather than let a bad value propagate. Exceptions are how your code talks to -its user. Use built-ins (`ValueError`, `KeyError`, `FileNotFoundError`) or define your own -where the domain warrants it. Never swallow with a bare `except`. +*Blocker.* Raise rather than let a bad value propagate. Use built-ins (`ValueError`, `KeyError`, +`FileNotFoundError`), or your own where the domain warrants it. Never swallow with a bare +`except`. ```python if height < 0.0: @@ -276,12 +253,11 @@ if height < 0.0: ``` ### Pure functions where you can -*Recommended.* Same input, same output, no side effects — easier to reason about and trivial -to test. Where side effects are necessary (files, databases, plots), keep them deliberate and -in few places. +*Recommended.* Same input, same output, no side effects — trivial to test. Where side effects +are necessary (files, databases, plots), keep them in few, deliberate places. ### Instance variables, not class variables -*Blocker.* A list defined in the class body is shared by every instance. Assign in `__init__`. +*Blocker.* A mutable value in the class body is shared by every instance. Assign in `__init__`. ```python class Toolbox: @@ -291,20 +267,19 @@ class Toolbox: ``` ### Type hints -*Recommended.* On public functions at minimum. They are hints, not enforcement — they exist -for the reader and the editor, until you add a [type checker](#type-checking-in-ci). +*Recommended.* On public functions at minimum. They are hints, not enforcement, until you add a +[type checker](#type-checking-in-ci). ```python def clip(values: list[int], *, threshold: int = 0) -> list[int]: ... ``` ### Keyword-only arguments -*Recommended.* One or two positional parameters is fine — that is the data the function -operates on. Everything after them is configuration, and belongs after a `*` so callers have -to name it. You can then reorder or add options without breaking anyone. - -Three or more positional parameters is a strong smell: `resample(df, 3, 0, True)` can't be -read at the call site, and nobody can safely change the order again. +*Recommended.* One or two positional parameters — the data the function operates on; +everything after is configuration and goes behind a `*`, so callers name it and you can add or +reorder options without breaking anyone. Three or more positional parameters is a smell: +`resample(df, 3, 0, True)` can't be read at the call site, and the order can never safely +change again. ```python def resample(data, freq, *, offset=0, dropna=True): ... @@ -312,8 +287,8 @@ resample(df, "1h", dropna=False) # ✓ the data and its frequency; the rest ``` ### Dataclasses for data -*Recommended.* Fields with type hints, a constructor, a useful `repr`, and equality by value -rather than by identity — for free. +*Recommended.* Fields with type hints, a constructor, a useful `repr`, and equality by value — +for free. ```python @dataclass @@ -323,9 +298,9 @@ class Interval: ``` ### Composed methods -*Recommended.* Each function does one identifiable task, and all operations inside it sit at -the same level of abstraction. Expect many small functions. A script split by comments is -asking to be split into functions. +*Recommended.* One identifiable task per function, all operations inside it at the same level +of abstraction. Expect many small functions. A script split by comments is asking to be split +into functions. ```python def main(): @@ -337,8 +312,7 @@ def main(): ### Comments say why, not what *Recommended.* A comment that restates the code is noise that goes stale. Write the ones that -capture what the code cannot say — the reason. If you need a comment to explain *what* is -happening, rename something instead. +capture the reason. If a comment is needed to explain *what* happens, rename something instead. ```python # Calculate the average temperature ← says nothing the code doesn't @@ -346,14 +320,12 @@ happening, rename something instead. ``` ### When a long signature is a smell -*Nice.* Many optional keyword arguments with sane defaults are perfectly Pythonic — see -`read_csv`, `plot`, or any sklearn estimator. The smell is not the total count (that's -[positional arguments](#keyword-only-arguments), which are a separate rule); it's when the -arguments are switches for **separate jobs** the function has absorbed. If half the signature -only applies when another argument is set, that's several functions wearing one signature. - -Then: group related parameters into a config dataclass, offer named presets, or split into -composable pieces that each do one thing. +*Nice.* Many optional keyword arguments with sane defaults are Pythonic — see `read_csv`, +`plot`, any sklearn estimator. The smell is not the count (that's +[positional arguments](#keyword-only-arguments), a separate rule) but arguments that switch +between **separate jobs** the function absorbed: if half the signature only applies when +another argument is set, that's several functions wearing one. Group them into a config +dataclass, offer named presets, or split into composable pieces. ```python plot_scatter(ax, x, y, show_density=True) # ✓ each does one job @@ -362,18 +334,18 @@ add_skill_table(ax, x, y, metrics=["bias"]) ``` ### Names carry meaning -*Recommended.* `n_freezing_days` over `n`, `FREEZING_POINT` over `0.0`. Renaming is the -cheapest refactoring there is. +*Recommended.* `n_freezing_days` over `n`, `FREEZING_POINT` over `0.0`. Renaming is the cheapest +refactoring there is. ## Design ### Composition over inheritance -*Recommended.* Composition is "has a", inheritance is "is a". Use inheritance only to -specialize behaviour — most of the time composition is the better fit. +*Recommended.* Composition is "has a", inheritance is "is a". Inherit only to specialize +behaviour; most of the time composition fits better. ### Encapsulate invariants -*Recommended.* A rule enforced only in `__init__` does not survive assignment. Use `_name` -plus a property when the invariant must hold. +*Recommended.* A rule enforced only in `__init__` does not survive assignment. Use `_name` plus +a property when the invariant must hold. ```python @property @@ -385,8 +357,8 @@ def name(self, value): self._name = value.upper() ### Don't reach into other classes *Blocker.* Classes talk through public APIs. Touching another object's `_private` attributes -couples you to its internals, and it will break. If you need something that isn't public, the -other class is missing a method — add it there. +couples you to its internals and will break. If what you need isn't public, the other class is +missing a method — add it there. ```python values = values[self.da.geometry.top_elements] # reaching in @@ -395,16 +367,16 @@ da = da.sel(layers="top") # ✓ ask it properly ### Pythonic over Java-like *Recommended.* Implement the dunder and get the language feature: `__len__` for `len(obj)`, -`__contains__` for `in`, `__iter__` for `for`, `__getitem__` for `obj[key]`. Your objects -should feel like the built-in types — `tb["hammer"]`, not `tb.getToolByName("hammer")`. +`__contains__` for `in`, `__iter__` for `for`, `__getitem__` for `obj[key]`. Aim for +`tb["hammer"]`, not `tb.getToolByName("hammer")`. ### Duck typing -*Recommended.* The caller cares that the methods exist, not what the type is. No base class or -interface required — that is what makes a scikit-learn transformer work. +*Recommended.* The caller cares that the methods exist, not what the type is. No base class +required — that is what makes a scikit-learn transformer work. ### Postel's law -*Recommended.* Be liberal in what you accept, conservative in what you send. Normalize input -types once, at the boundary. Pydantic does this for you. +*Recommended.* Liberal in what you accept, conservative in what you send. Normalize input types +once, at the boundary. Pydantic does this for you. ```python def process(number: int | str | float) -> int: @@ -428,7 +400,7 @@ pip install https://github.com/DHI/my_library/archive/main.zip ``` ### Docstrings, numpy format -*Recommended.* On every public function and class. Written once, read in `help()`, in the +*Recommended.* On every public function and class. Written once; read in `help()`, in the editor tooltip, and on the generated API site. Numpy format is the DHI default — set `docstring_style: "numpy"` in mkdocs, since the default is google. @@ -449,28 +421,25 @@ def remove_outlier(data: pd.DataFrame, column: str, threshold: float = 3) -> pd. ``` ### Examples that are tested -*Nice.* Documentation that is wrong is worse than documentation that is missing. `doctest` -runs the examples in your docstrings. For prose pages, [Quarto](https://quarto.org/) executes -every snippet as part of the build, so the docs cannot ship broken — the build fails first. +*Nice.* Wrong documentation is worse than missing documentation. `doctest` runs the examples in +your docstrings; [Quarto](https://quarto.org/) executes every snippet in prose pages at build +time. Either way broken docs fail the build instead of shipping. ```bash -python -m doctest -v add.py +uv run pytest --doctest-modules src ``` ### Published API documentation -*Recommended.* `mkdocs` + `mkdocstrings` + GitHub Pages, at -`https://dhi.github.io//`. [Quarto](https://quarto.org/), -[Great Docs](https://github.com/machow/great-docs) (which wraps Quarto) and -[zensical](https://zensical.org/) are viable alternatives. - -A private repository can have access-controlled Pages on GitHub Enterprise — use that when the -site should stay internal, rather than relying on the URL not being found. +*Recommended.* `mkdocs` + `mkdocstrings` + GitHub Pages, at `https://dhi.github.io//`. +[Quarto](https://quarto.org/), [Great Docs](https://github.com/machow/great-docs) (which wraps +Quarto) and [zensical](https://zensical.org/) are alternatives. Internal-only sites go on +access-controlled Pages on GitHub Enterprise — not on an unlisted URL. ## Automation ### CI on every push and pull request -*Blocker.* A workflow in `.github/workflows/` that installs and runs the tests. This is what -solves "it works on my machine". +*Blocker.* A workflow in `.github/workflows/` that installs the package and runs the tests. +This is what solves "it works on my machine". ```yaml on: @@ -489,35 +458,34 @@ jobs: ``` ### Lint and format with ruff -*Recommended.* There is no reason not to. One binary, no configuration required, and it -replaces flake8, black and isort at once. `ruff check` finds unused imports, undefined names -and dead variables — usually typos, sometimes bugs. `ruff format` ends style arguments. Run -both in CI. +*Recommended.* One binary, no configuration required, replaces flake8, black and isort. +`ruff check` finds unused imports, undefined names and dead variables — usually typos, sometimes +bugs; `ruff format` ends style arguments. Run both in CI. ```bash ruff check . ruff format --check . ``` ### Type checking in CI -*Nice.* [Type hints](#type-hints) are not enforcement — a type checker is. Run `mypy` (or -`ty`) in CI on the package, not the tests, and turn it on for new code before old. It also -catches uses of anything you have marked -[`@deprecated`](#deprecate-before-removing). +*Nice.* [Type hints](#type-hints) are not enforcement — a type checker is. Run `mypy` (or `ty`) +on the package, not the tests, and turn it on for new code before old. It also catches uses of +anything marked [`@deprecated`](#deprecate-before-removing). ```bash uv run mypy src --enable-error-code=deprecated ``` ### A task runner -*Nice.* One source of truth for how to run the project's tools, and the fastest onboarding -document there is — for people and for coding agents. Prefer -[`just`](https://just.systems) (`uv tool install rust-just`): a single cross-platform binary, -`Makefile`-like syntax, and `just --list` documents itself. `make` works too, but it is not -installed on Windows and is a build tool pressed into service as a task runner. The +*Nice.* One source of truth for how to run the project's tools — the fastest onboarding +document there is, for people and for coding agents. Prefer [`just`](https://just.systems) +(`uv tool install rust-just`): a single binary, `Makefile`-like syntax, `just --list` documents +itself. `make` works, but is absent on Windows. The [DHI template](https://github.com/DHI/template-python-library) ships a `justfile`. ```just -check: lint typecheck test # justfile +set windows-shell := ["powershell.exe", "-NoLogo", "-Command"] + +check: lint typecheck test lint: uv run ruff check src test: @@ -525,26 +493,22 @@ test: ``` ### Test the matrix -*Nice.* If you claim to support Windows and Python 3.10, test on Windows and Python 3.10. - -Test what you claim and no more. CI is not free — every cell costs minutes on every push. An -[application](#libraries-loose-applications-pinned) has one deployment target, so one cell is -the honest matrix; a library that others install needs the range it advertises in -`requires-python`. +*Nice.* Test what you claim to support, and no more — every cell costs minutes on every push. +An [application](#libraries-loose-applications-pinned) has one deployment target, so one cell is +the honest matrix; a library needs the range it advertises in `requires-python`. ```yaml strategy: matrix: os: [ubuntu-latest, windows-latest] - python-version: ["3.10", "3.13"] + python-version: ["3.11", "3.13"] ``` ## Release ### Tag every release *Recommended.* An annotated `vX.Y.Z` tag, pushed. It is what makes "which commit is 1.2.0?" -answerable a year later, and what lets you diff two releases. Just do it — it costs one -command. +answerable a year later, and what lets you diff two releases. ```bash git tag -a v1.2.0 -m "v1.2.0" @@ -554,10 +518,7 @@ git push --tags ### Publish from a tag or a release *Recommended.* Let a workflow build and publish; never upload from your laptop. Use [Trusted Publishers](https://docs.pypi.org/trusted-publishers/) so there are no secrets to -manage. - -Trigger on the tag, or on a published GitHub release — either works with Trusted Publishers. -The release gives you somewhere to put release notes; the tag is one step fewer. +manage. Trigger on the tag, or on a published release if you want somewhere to put notes. ```yaml on: