Add consider-narrowing-parameter check (R1738) - #11236
Draft
Pierre-Sassoulas wants to merge 7 commits into
Draft
Conversation
Emitted when a function or method parameter is only ever used to access a single attribute, meaning the function could accept that attribute's value directly (stamp coupling that could be data coupling). The new ``ignored-function-names`` option excludes functions whose signature is imposed by a convention or a protocol (e.g. visitor callbacks). The message is disabled on our own code base for now: it raises 86 legitimate suggestions that will be cleaned up separately.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11236 +/- ##
==========================================
+ Coverage 96.35% 96.37% +0.02%
==========================================
Files 178 178
Lines 19932 20068 +136
==========================================
+ Hits 19205 19341 +136
Misses 727 727
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
The primer showed hits on 'def fix(self=self)' closure captures and on module-level functions destined to be attached to a class: these follow the method convention even outside a class body. Also document 'ignored-function-names' as the right knob for pytest test suites, where parameters are fixtures injected by name (38% of primer hits were in test files).
Two changes to cut false positives found by the primer: - A parameter whose attribute is accessed only once is no longer reported: short adapters implementing a duck-typed interface (like 'filter(record)' for the logging framework in astropy) look exactly like a narrowing candidate but their signature is imposed. - A function referenced anywhere in its module without being called (passed as a callback, registered in a container...) is no longer reported: its signature belongs to whoever ends up calling it. Self-hits on our own code base drop from 86 to 27, and 5 of the 8 functional test files touched by the initial commit no longer need their disable.
This comment has been minimized.
This comment has been minimized.
… hit Reviewing each of the 246 primer messages one by one surfaced three hits where applying the suggestion would change behavior, each with its own mechanism. Three new guards: - an access inside a 'try' whose handler would catch AttributeError cannot be hoisted past the guard (pytest's '_getcrashline'); - a method overridden by a subclass of the same module cannot be narrowed without breaking the polymorphic call sites (ansible's '_check_results'); - when an exit point precedes every access, narrowing would make a side-effectful property access unconditional at the call site (music21's 'setEditorial' and its lazily-created '.editorial'). The check is also restricted to private and nested functions by default - the review showed public hits are dominated by deliberate design (method-family symmetry, domain objects) and framework hooks. The new 'suggest-narrowing-public-parameters' option restores the wider scope.
Bare 'except:', tuple handlers, dotted 'builtins.AttributeError' and the 3.11+ 'except*' form all guard an attribute access the same way.
Contributor
|
🤖 Effect of this PR on checked open source code: 🤖 Effect on astropy: New messages: Details
Effect on music21: New messages: Details
Effect on pygame: New messages: Details
Effect on ansible: New messages: Details
Effect on django: New messages: Details
Effect on pandas: New messages: Details
Effect on pytest: New messages: Details
Effect on astroid: New messages: Details
Effect on sentry: New messages: Details
Effect on black: New messages: Details
Effect on home-assistant: New messages: Details
Effect on psycopg: New messages: Details
This comment was generated for commit 575bd39 |
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.
Type of Changes
Description
Emitted when a function or method parameter is only ever used to access a single attribute, meaning the function could accept that attribute's value directly (stamp coupling that could be data coupling).
The new
ignored-function-namesoption excludes functions whose signature is imposed by a convention or a protocol (e.g. visitor callbacks).Want to see the primer for this and what poeple think basically.