Adopt pylint, df12-python-lints, and stricter Ruff for Python sources - #542
Draft
leynos wants to merge 4 commits into
Draft
Adopt pylint, df12-python-lints, and stricter Ruff for Python sources#542leynos wants to merge 4 commits into
leynos wants to merge 4 commits into
Conversation
Netsuke's Python surface — the helper scripts under `scripts/` and the workflow contract tests — was linted only in part. `make spelling-helper-test` ran Ruff over eleven named files in isolated mode, and nothing at all covered `scripts/hoist_binstall_*.py` or `tests/workflow_contracts/`. Replace the root `ruff.toml` with a `pyproject.toml` carrying the stricter shared configuration: preview mode, the ASYNC, D, and DOC rule groups, the NumPy docstring convention, the banned deprecated-typing-generics table, and the mccabe and Pylint design limits. The file deliberately declares no `[project]` table and no `[build-system]`, so nothing can build a Python distribution from this repository and uv never treats it as a Python project. Add three lint passes to `make lint` via a new `lint-python` target: a repository-wide `ruff check`, Pylint on PyPy through the pylint-pypy shim, and the df12 house lints (both the main message set and the separate future-annotations pass) on CPython 3.14 through `uv tool run`, so no contributor needs a project virtual environment. `ambrleaks` scans `tests` for unredacted snapshot values. `make check-fmt` and `make fmt` gain the pinned `ruff format` pass. The existing isolated Ruff runs gain `--preview` so they agree with the repository configuration, and the plain `uv run` invocations gain `--no-project` so the new `pyproject.toml` cannot pull a virtual environment into a Rust contributor's working tree.
Bring every helper script, workflow contract test, and the vendored Cyclopts stub up to the newly enforced rule set. The substantive changes are the facade in `scripts/typos_rollout.py` and the rollback path in `scripts/hoist_binstall_archives.py`. The facade now re-exports by name rather than by assignment, declares `__all__`, and drops four re-exports (`_CacheTargets`, `_RemoteResponse`, `_HttpsRedirectHandler`, and `_HTTPS_OPENER`) that nothing referenced. The rollback path chains the combined failure group from the rollback failure instead of suppressing the cause outright, which keeps the traceback honest and satisfies the blind-except rule without weakening the deliberate `BaseException` catch. The documented exception type there was `ExceptionGroup` where the code raises `BaseExceptionGroup`; the docstrings now match. The remainder is mechanical: exception messages bound to locals, deprecated typing generics and explicit member imports replaced with module imports, annotation-only imports moved into type-checking blocks, a redundant `...` in a Protocol body removed, an `operator.itemgetter` in place of a lambda, and full Returns and Raises sections on `_HttpsRedirectHandler.redirect_request`. Four rules are suppressed at the site with a recorded reason: `S404`, `S603`, and `S607` where the helpers run a fixed `git` argv without a shell; `S310` where the transport entry point is compared or forwarded rather than opened, and where the HTTPS guard S310 asks for sits on the line above; and `SIM115` where the temporary path must be bound before the `with` block so the `finally` can remove it after the rename. `DOC502` is disabled repository-wide, because these helpers deliberately document the exceptions they propagate.
Bind the whole subject in the string branch of the archive-resolution match so the pattern reads as a capture rather than a positional sub-pattern, and test for an empty glob result by truthiness rather than by comparison with a list literal. Trim `typos_rollout.py` and `typos_rollout_http.py` back under the 400-line module cap that the previous commit's documentation additions had pushed them over. The facade now marks its unused re-exports with redundant `X as X` aliases instead of carrying an `__all__` list, which says the same thing in one line each.
Give every assertion in the spelling-rollout suites a failure message, so a red test names the contract it broke rather than printing a bare comparison. Consolidate the three substring probes in the gawk-staging contract test into one assertion over the required fragments. The test still covers all three, reports every missing fragment at once instead of stopping at the first, and no longer reads as the repeated-substring pattern a snapshot would serve better. Declare the two callable aliases in `typos_rollout_http` with `type` statements, and move the explanations for the remaining `noqa` directives onto the suppressed lines themselves, which is where the suppression lint looks for them.
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Contributor
Reviewer's GuideAdopts a unified Python linting stack (Ruff, Pylint, df12-python-lints, ambrleaks) via a new pyproject.toml and Makefile wiring, then updates Python scripts and tests to satisfy the stricter rules while making a few small behavioral and clarity improvements in typos rollout and binstall hoist logic. Sequence diagram for hoist rollback BaseExceptionGroup chainingsequenceDiagram
participant Caller
participant hoist
participant _move_all
participant _rollback_completed_moves
Caller->>hoist: hoist(dist_dir, staging_config, manifest, version)
hoist->>_move_all: _move_all(dist_dir, located)
alt move succeeds
_move_all-->>hoist: return
hoist-->>Caller: return
else move fails
_move_all->>_move_all: failure = caught OSError
_move_all->>_rollback_completed_moves: _rollback_completed_moves(completed)
alt rollback succeeds
_rollback_completed_moves-->>_move_all: return
_move_all-->>hoist: raise failure
else rollback fails
_rollback_completed_moves-->>_move_all: raise rollback_failure
_move_all->>_move_all: raise BaseExceptionGroup(msg, [failure, rollback_failure]) from rollback_failure
_move_all-->>hoist: raise BaseExceptionGroup
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates Failed
(1 file with Large Method)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| test_typos_rollout_refresh.py | 1 advisory rule | 10.00 → 9.60 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Comment on lines
+167
to
+169
| assert timeout == pytest.approx(30.0), ( | ||
| "the network boundary was called without the 30s timeout" | ||
| ) |
There was a problem hiding this comment.
❌ New issue: Large Method
test_http_refresh_scopes_validators_and_preserves_newer_cache has 72 lines, threshold = 70
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
1 participant
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.
Summary
Netsuke's Python surface — the helper scripts under
scripts/and the workflowcontract tests under
tests/workflow_contracts/— was linted only in part.make spelling-helper-testran Ruff in isolated mode over eleven named files,and nothing at all covered
scripts/hoist_binstall_*.pyortests/workflow_contracts/. Nothing ran Pylint, and the repository had nopyproject.toml.This change adopts the estate-wide Python lint stack and clears every finding it
raises.
pyproject.tomlreplacesruff.toml. It carries the stricter sharedRuff configuration — preview mode, the
ASYNC,D, andDOCrule groups,the NumPy docstring convention, the banned deprecated-typing-generics table,
and the mccabe and Pylint design limits — plus the
[tool.pylint.*]messagecontrol tables. It deliberately declares no
[project]table and no[build-system]: Netsuke publishes a Rust crate, so nothing here may build aPython distribution, and uv never treats the repository as a Python project.
make lintgains alint-pythonprerequisite that runs a repository-wideruff check, Pylint on PyPy through the pylint-pypy shim, the df12 houselints and their separate future-annotations pass on CPython 3.14, and
ambrleaksovertests. Every pass runs throughuv tool run, so a Rustcontributor never needs a project virtual environment.
make check-fmtandmake fmtgain the pinnedruff formatpass.are suppressed at the site with a recorded reason, and one is disabled
repository-wide with a recorded reason; see Notes.
Review walkthrough
Start with the configuration, then the two substantive source changes, then the
mechanical remainder.
pyproject.toml— the new configuration. Note the header comment explaining the absent
[project]table, and theDOC502entry inextend-ignorewith itsrationale.
Makefile— the
RUFF,PYLINT,DF12_PYLINT,DF12_FUTURE_ANNOTATIONS, andAMBRLEAKSdefinitions, the newlint-pythontarget, and the--no-projectguards added to the plain
uv runinvocations so the newpyproject.tomlcannot pull a virtual environment into a working tree.
scripts/typos_rollout.py— the facade now re-exports by name rather than by assignment, marks the
re-exports it does not itself use with redundant
X as Xaliases, and dropsfour re-exports (
_CacheTargets,_RemoteResponse,_HttpsRedirectHandler,_HTTPS_OPENER) that nothing referenced.scripts/hoist_binstall_archives.py— the rollback path chains the combined failure group from the rollback
failure instead of suppressing the cause outright. The group still carries
both exceptions, the deliberate
BaseExceptioncatch is unchanged, and thedocstrings now name
BaseExceptionGroup, which is what the code raises.tests/workflow_contracts/ci_lint_test.py— the three substring probes in the gawk-staging test are consolidated into
one assertion over the required fragments. Coverage is unchanged, and a
failure now names every missing fragment rather than stopping at the first.
scripts/tests/test_typos_rollout_refresh.pyand
scripts/tests/test_typos_rollout_hardening.py— every assertion gains a failure message naming the contract it broke.
scripts/typos_rollout_http.py—
typestatements for the two callable aliases, and full Returns and Raisessections on
_HttpsRedirectHandler.redirect_request.typing generics and explicit member imports replaced with module imports,
annotation-only imports moved into type-checking blocks, a redundant
...removed from a Protocol body, and
operator.itemgetterin place of a lambda.Validation
Run sequentially against a shared build cache.
make check-fmtcargo fmt --checkclean, 20 Python files already formattedmake lintmake typecheckmake testmake test-workflow-contractsmake markdownlinttyposclean, 76 Markdown files, 0 errorsNotes
DOC502is disabled repository-wide, with the reason recorded inpyproject.toml. The rule rejects anyRaisesentry the function does notraise with a literal
raisestatement, which would have forced these helpersto delete twenty-one accurate entries documenting the exceptions they
propagate from the filesystem, TOML, and subprocess calls they make. That
propagation is the contract a caller must handle.
DOC501remains enabled, sothe stale-documentation risk is still covered from the other direction.
suppression lint requires:
S404,S603, andS607inscripts/typos_rollout_check.pyandscripts/tests/test_typos_rollout_check.py, where the only subprocess is afixed
gitargv with no shell.gitresolves throughPATHdeliberately,so the check uses the same Git the developer or CI runner invokes.
S310inscripts/typos_rollout.py, where the transport entry point iscompared and forwarded but never opened, and in
scripts/typos_rollout_http.py, where the HTTPS scheme audit S310 asks foris the guard on the preceding lines and
_HttpsRedirectHandlerre-appliesit to every redirect target.
SIM115inscripts/typos_rollout_cache.py, where the temporary path mustbe bound before the
withblock so thefinallycan remove it after therename. The stream is closed by that
with.--previewadded to the isolated Ruff format check inspelling-helper-test. The gate stays--isolated, so the spelling helpersremain provably self-contained, but the flag must match the
preview = truein
pyproject.tomlor the two formatters disagree on how to lay out a soledictionary argument.
max-module-lines = 400putscripts/typos_rollout.pyandscripts/typos_rollout_http.pyover the limitonce the required documentation was added; both were within a dozen lines of
the cap already. They were trimmed back under it rather than the cap being
raised, but both are now close enough that the next addition to either will
need a split.
C9112is a no-op today. The df12 future-annotations pass readspy-version = "3.13"frompyproject.toml, and the message only fires on a3.14 or later baseline. It is wired in now so it starts reporting the moment
the baseline moves.
.gitignoregains.venv/anduv.lock, so a strayuv runwithout--no-projectcannot leave either behind.Summary by Sourcery
Adopt shared Python linting and formatting configuration via pyproject.toml and integrate it into the existing Makefile workflows, updating helper scripts, tests, and documentation to comply with the new lint rules.
Enhancements:
Documentation:
Tests:
Chores: