Restore pyflakes in the ruff gate and correct the Python floor - #743
Open
bschwedler wants to merge 9 commits into
Open
Restore pyflakes in the ruff gate and correct the Python floor#743bschwedler wants to merge 9 commits into
bschwedler wants to merge 9 commits into
Conversation
posit_bakery/error.py defines three exception classes that subclass the builtin ExceptionGroup at import time, and ExceptionGroup only exists from Python 3.11 (PEP 654). The package nonetheless declared requires-python = ">=3.10" and advertised a 3.10 trove classifier, so pip would happily install it on 3.10, where importing posit_bakery.error raises NameError immediately -- and cli/build.py imports it, so the CLI could not start at all. Nothing here actually supported 3.10; the dev dependency on great-docs was already gated on python_version >= '3.11'. Correct the declaration rather than add a backport. Also sets ruff's target-version to match, which is what let this go unnoticed: at py310 ruff reports the ExceptionGroup references as undefined names, and those reports were being suppressed by the lint configuration fixed separately in this branch.
patch_version() was annotated `-> "ImageVersion"`, but ImageVersion was never imported into config.py, so the forward reference pointed at nothing: get_type_hints() on the method raised NameError, and neither mypy nor an IDE could resolve it. Nothing evaluates the annotation at runtime today -- BakeryConfig is a plain class, not a pydantic model -- so this was a wrong signature rather than a crash. Other cross-module quoted annotations in this package are backed by a TYPE_CHECKING block, but every one of those exists to break a specific circular import. There is none here: nothing under config/image/ imports config.config, and config.py already imports Image from that same package, which loads ImageVersion as a side effect. Adding it to the existing import binds an already-loaded name and executes no new module code, so a deferred import would only imply a cycle that does not exist.
Nine top-level CLI handlers used a bare `except:`, which catches BaseException -- so Ctrl-C during a create, remove, get or ci command was swallowed and reported as "Failed to ..." with exit code 1, rather than interrupting. SystemExit was caught the same way. Narrowing to Exception leaves every ordinary failure on the existing path: typer.Exit derives from RuntimeError, so the handlers that re-raise it still behave identically, and so does every Bakery error. Only KeyboardInterrupt, SystemExit and GeneratorExit change, and for all three propagating is the correct behavior.
Regenerates uv.lock after requires-python moved to >=3.11: the python_full_version < '3.11' resolution branch is dropped and the remaining markers collapse accordingly. `uv lock --check` is clean against the updated pyproject.
Twenty-seven string literals were marked as f-strings but interpolate nothing (F541). Several are the opening line of an error message that is appended to afterwards, so the prefix reads as intentional at a glance and is not; the rest are plain leftovers. No behavior change: an f-string with no replacement fields evaluates to the same str.
Six names were defined twice in the same scope, so Python bound the second and the first never ran. Each case differed, so each is handled on its own terms rather than by deleting the duplicate: - test_image_target.py: two unrelated tests both named test_build_args. The shadowed one is parametrized and covers dependency-to-build-arg resolution across matrix cases; the live one checks the build property dict. Renamed the former to test_build_args_from_dependencies, which restores roughly sixteen parameter cases that had never executed. - test_spec.py: the shadowed test was the stricter of the pair, asserting the "version must not be empty" message where its replacement only asserted ValidationError. Removed the weaker later duplicate so the strict one runs again. - test_bake.py: the shadowed test called get_expected_plan() with the pre-refactor single argument and would fail if run. Deleted. - conftest.py: cli_tmpcontext was redefined as a byte-identical copy of cli_cwd_context, registering the same step string twice and hiding the real cli_tmpcontext. Deleted the copy. - test_ci.py: three distinct @then steps all named check_log_metadata_targets. pytest-bdd resolves by step string so all three ran, but the names collided; renamed two to match their steps. All previously shadowed assertions pass.
Twenty-three imports were dead. One was not: test/conftest.py imports posit_bakery.image.image_target inside an autouse fixture purely so the module is loaded before mocker.patch resolves its patch targets from strings. That one keeps a noqa and a comment explaining why the name looks unused. Ruff declines to autofix inside __init__.py on the assumption that imports there may be re-exports, so the unused `warnings` in the dgoss plugin was removed by hand after confirming the module is a CLI entry point rather than a re-export barrel.
Most of the twelve unused locals were assignments whose value was never the point -- constructor calls inside pytest.raises blocks, or leftovers from removed assertions (original_path duplicated old_path on the very next line). Two were not cosmetic: - test_oras.py: test_no_sources_is_success computed a result and never asserted on it, so it checked only that subprocess was not spawned, not the success its name claims. Added the missing assertion. - wizcli/suite.py: parse_err captured a report-parse failure that nothing ever read. The failure is still logged, but when wizcli itself exits 0 a malformed report is not surfaced as an error. Removing the dead variable does not change that; it is left as-is rather than fixed silently inside a lint cleanup. For the two module-level imports outside the header: test/conftest.py's is deliberate and already documented -- it must follow discover_plugins() -- so it takes a noqa. test/plugins/test_registry.py's had no such constraint or comment, and moving it to the header leaves the suite green.
ruff's `select` replaces its default rule set rather than adding to it, so configuring only T201 disabled everything else -- including pyflakes. That is why a call to an _emit_summary() that was never defined passed lint and reached CI, and why several tests shadowed by duplicate definitions went unnoticed for as long as they did. The set is written out explicitly instead of switching to extend-select. Inheriting ruff's default would enable roughly four hundred rules here, producing several hundred findings unrelated to this change, and would let a future ruff upgrade alter what CI enforces with no commit to point at. Listing the rules means the gate only moves when someone moves it. Verified both ends: a reference to an undefined name is now reported, and print() is still reported.
ianpittwood
reviewed
Aug 11, 2026
ianpittwood
left a comment
ianpittwood
left a comment
Contributor
There was a problem hiding this comment.
Are the key changes in posit-bakery/pyproject.toml?
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
2 participants
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.
Ruff's
selectreplaces the default rule set. It does not add to it. A configuration with onlyT201therefore turns off every other rule. This is why a call to an_emit_summary()that did not exist passed the lint step and reached CI.This change writes the rule set out in full. It does not use
extend-select. Ruff's default enables about four hundred rules and gives 629 findings that are unrelated to this change. An explicit list also means that a ruff upgrade cannot change what CI enforces without a commit.The 80 findings included two defects. The declared Python floor was wrong, because
error.pycannot import on Python 3.10. Four tests never ran, because duplicate definitions shadowed them. Each shadowed test is handled on its own terms, not by deletion of the duplicate. In one case the shadowed test was the stricter test of the pair.One finding stays open.
wizcli/suite.pyassigned aparse_errvalue that nothing reads. The suite logs a malformed report but does not surface it when wizcli exits with code 0. This change removes the dead variable only. A change to when scans fail does not belong in a lint change.Closes #742