Lint the python and hold the package's layering in CI - #41
Merged
Conversation
The translator imported `strip_inline_comment` without calling it: the name was reachable only because the parser tests still went through the translator's namespace, from before the parser moved to `yamlite`. Point those assertions at `yamlite` and drop the import.
Nothing has been checking the Python for unused imports, undefined names, or shadowed builtins -- the kinds of mistake that survive a green test run because no test happens to reach the line. Ruff's default rules only: pyflakes plus the pycodestyle checks about statement shape, and none of the whitespace, line-length, or import-order ones. A linter that reflows untouched files buries the diff that is actually under review, so `ruff format` stays out of the repo entirely. Test modules are exempt from the import-placement rule; each one puts the repo root on sys.path first so it runs standalone as well as under discovery. The launcher entry points have to be named explicitly. Ruff finds files by extension and those carry none, being commands rather than modules -- which would have left the only python doing sys.path surgery as the only python nobody checks. CI pins the ruff version: an unpinned linter turns a green branch red on someone else's release day.
Two properties kept the package usable on both sides of the container boundary, and nothing was checking either. The first is that the modules are layered: `yamlite` is a leaf both sides import, the tongs modules build on each other in one direction, and the anvil modules sit on top of tongs. A cycle is not a build failure -- python resolves most of them, and the ones it cannot surface as a half-initialised module at startup, far from the import that closed the ring. So the graph is read out of the source with `ast` and the whole ring is reported, not just the edge that closed it. Reaching into a package counts as depending on it, since python runs the `__init__` on the way in; an importer already inside the package does not, because an `__init__` importing its own children is the arrangement here. Imports that do not run at import time -- inside a function, or behind `if TYPE_CHECKING:` -- are not edges at all. Those are the two standard ways to break a cycle, and a check that failed the fix it asked for would be worse than none. The second property is that only the entry-point shims load python from a file path. That is what the flat layout did, and it costs what a package buys: a module loaded that way is a second copy under a name nothing imports, invisible to a linter and unmockable through the usual seams. The status-line seeder's test is the one standing exception -- the seeder ships into the image as a standalone script, so there is no module path to reach it by -- and the exemption is itself asserted on, so it cannot quietly outlive its reason. Both scans have to say what they covered. The import graph refuses to skip a file it cannot parse, because everything it reads is ours. The path-loading scan walks the whole checkout and so must tolerate whatever a developer's tree holds, which means it needs a floor: it is required to keep finding a shim, a package module, and a test. The directories it skips are matched by place, not by name, so a package directory sharing a name with a container store is still read.
The rules were only in the tests. Say what the levels are, so a contributor learns the shape from the prose rather than from a failure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three checks that nothing was doing, plus the one real defect the first of them found.
Ruff, as a linter only
make lintrunsruff checkover every Python file in the repo, with a pinned version in its own CI job. The rule set is ruff's defaults — pyflakes plus the pycodestyle checks about statement shape — and stops there deliberately: line length, import order, and whitespace are the author's, so switching the linter on does not reflow files a change never touched.ruff formatis not part of this repo.The launcher entry points had to be named explicitly in the config. Ruff finds files by extension and
bin/run-anvil,bin/tongs, andbin/git-guardcarry none, being commands rather than modules — which would have left the only python doingsys.pathsurgery as the only python nobody checks. Their existing# noqa: E402comments were dead until now.Turning it on surfaced exactly one thing:
swarmforge/agents/translate.pyimportedstrip_inline_commentwithout calling it. The name was reachable only because the parser tests still went through the translator's namespace, from before the parser moved toyamlite. Those assertions now callyamlitedirectly and the import is gone. It is the only source-file change in this PR.The package's imports stay acyclic
The package serves both sides of the container boundary, and it only does that because its modules are layered:
yamliteis a leaf either side can import, the tongs modules build on each other in one direction, and the anvil modules sit on top of tongs. Nothing held that shape.A cycle is not a build failure — python resolves most of them, and the ones it cannot surface as a half-initialised module at startup, far from the import that closed the ring. So the graph is read straight out of the source with
ast, and the whole ring is reported rather than the edge that closed it.Two decisions worth knowing when it fires:
__init__on the way in. An importer already inside that package does not count, since an__init__importing its own children is how both subpackages are arranged. Without this the one shape that actually strands a half-initialised module — a ring closing through an__init__— goes unreported.if TYPE_CHECKING:. Those are the two standard ways to break a cycle, and a check that failed the fix it just demanded would be worse than no check.Loading a module by file path stays in
bin/That is what the flat layout did before there was a package, and every consumer carried its own copy of the block deriving the path. What it loads is a second module under a name nothing imports — invisible to a linter, unmockable through the usual seams, and silently a different object from the one the rest of the tree holds.
The entry-point shims stay exempt, because resolving the checkout's path is the only thing they do. So does the status-line seeder's test: the seeder ships into the image as a standalone script rather than inside the package, so there is no module path to import it by. That exemption is itself asserted on — an entry whose file stops loading by path fails the build rather than quietly widening the rule.
Both scans say what they covered
A guardrail that silently scans nothing reports everything clean. The import graph refuses to skip a file it cannot parse, because everything it reads is ours. The path-loading scan walks the whole checkout, so it has to tolerate whatever a developer's tree holds — a fifo, a root-owned file a container wrote, a script for another interpreter — and therefore carries a floor: it must keep finding a shim, a package module, and a test. Directories it skips are matched by place rather than by name, so a package directory sharing a name with a container store is still read.
Checks
make lintclean, at the version CI pins.