DM-55493: scope PLR0917 to exclude FastAPI handlers - #351
Closed
jonathansick wants to merge 1 commit into
Closed
Conversation
ruff 0.16.0 stabilized PLR0917 (too-many-positional-arguments) out of preview. Since ruff-shared.toml selects ALL, it now fires on every FastAPI route handler, which legitimately takes more than five parameters (path and query parameters plus Depends injections) and is only ever called by FastAPI itself, by keyword. Add PLR0917 to the existing handlers per-file-ignores rather than the global ignore list, so the rule keeps firing on ordinary internal APIs.
rra
reviewed
Jul 27, 2026
rra
left a comment
Member
There was a problem hiding this comment.
I like this rule and would rather not exclude it. I want to leave it active in the shared Ruff configuration so that it affects my packages.
FastAPI route handlers are regular functions and sometimes it's useful to call them from, e.g., another route handler. In those cases, long argument lists should be passed by keyword for all the reasons that we would do so elsewhere. I think it's fairly easy to just add *, to the start of the argument list for FastAPI route handlers, which satisfies this diagnostic.
Member
Author
|
Ok good call. I'll adapt the signatures on my handlers. |
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.
Motivation
ruff 0.16.0 (2026-07-23) stabilized
PLR0917(too-many-positional-arguments) out of preview. Becauseruff-shared.tomlusesselect = ["ALL"], the rule armed automatically in every repo that picked up the new ruff — no config change required.PLR0917is a good rule for ordinary internal APIs: a call site passing six bare positional arguments really is hard to read, and the fix (make some of them keyword-only) really is an improvement.It is not a good rule for FastAPI route handlers. A handler routinely exceeds five parameters — path parameters, query parameters, and one or more
Depends(...)injections — and that signature is not a call-site ergonomics problem, because our code never calls handlers at all. FastAPI calls them, by keyword, from the resolved dependency graph. So inhandlers/the rule protects nothing and is pure noise.Change
Adds
PLR0917to the per-file-ignore entries that already scope rules to the handlers package, in the fastapi_safir_app templates only:square_pypi_packageis deliberately untouched — it has nohandlers/.Why a scoped ignore and not a global one
ruff-shared.tomlalready globally ignores the sibling rulePLR0913(too-many-arguments), with the comment "factory pattern uses constructors with many arguments". It would be easy to assumePLR0917belongs next to it. It doesn't — they are different concerns:PLR0913counts all arguments. Our factory/constructor pattern legitimately has many, everywhere in the codebase, so a global ignore is the right call.PLR0917counts only positional arguments — i.e. it is a complaint about call sites, and it is silenced by making parameters keyword-only. That is usually the right fix, so the rule should keep firing on ordinary internal APIs and drive better signatures there.FastAPI handlers are the narrow case where the fix is neither available nor wanted (the signature is an interface contract with FastAPI, not with us). Hence a path-scoped exemption rather than a blanket entry in
ignore.Glob choice
I followed the file's existing convention rather than introducing a new
"**/handlers/**"key: the config already carries the handlers path as a pair of globs,"src/*/handlers/**"and"*/src/*/handlers/**", mirroring the same pairing used fortests/**and*/tests/**.PLR0917was added to both so the entries stay symmetric. Happy to switch to a single**/handlers/**if maintainers would rather consolidate — but that felt like a separate cleanup touching several unrelated entries.Validation
Applied to all three copies (
{{cookiecutter.name}},example,example-uws); the three files remain byte-identical (same md5).templatekit check(the CI job — re-renders every example with scons and diffs against the committed state) passes:✅ Passed!All five
ruff-shared.tomlfiles parse as TOML; confirmedPLR0917is not present in any globalignorelist.Behavior verified against ruff 0.16.0 specifically (the repo's pinned pre-commit ruff is older and does not know these rules), using a scratch project that extends this config and contains the same 7-positional-argument function in two places:
src/myapp/handlers/external.pysrc/myapp/services/thing.pymainuvx ruff@0.16.0 check --select PLR0917,ISC004 .inside the renderedexample/app:All checks passed!Two honest caveats on that command:
{{cookiecutter.name}}/pyproject.toml, which contains Jinja. That is a property of a cookiecutter repo, not of this change.example-uws/it reports a pre-existinginvalid-syntaxinsrc/exampleuws/dependencies.py:15(a scaffold stub: a bare*,separator followed only by comments). That file is byte-identical tomainand is untouched here.No changelog fragment: this repo has no
changelog.d/at its root, and a lint-config adjustment is not user-visible in the way scriv fragments track.Ticket: DM-55493