-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ignore cached_property in method-hidden check (#8753) #8758
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8758 +/- ##
==========================================
+ Coverage 95.82% 95.83% +0.01%
==========================================
Files 173 173
Lines 18386 18526 +140
==========================================
+ Hits 17618 17755 +137
- Misses 768 771 +3
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty nice already ! I suggested a test case to make sure we use the qname / inference, not sure if this already work or how necessary this is.
pass | ||
|
||
@functools.cached_property | ||
def _protected_two(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no actual definition cached_property
, so safe_infer
returns UnInferable
, and this line did not detect.
By adding the actual definition, this line will be detected, any good ideas?
pylint/pylint/checkers/classes/class_checker.py
Lines 1261 to 1262 in 1ef3b0b
inferred = safe_infer(decorator) | |
if not inferred: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the expected behavior π We should not raise when something is uninferable (unless the real functools.cached_property is always uninferable).
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking the remark into account. Let's wait for another maintainer opinion regarding aliasing before investing too much time in it :) If it's important then we should use qname
(qualified name, i.e. builtins.property
) instead.
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
The docs on adding a changelog are here (you can install the dev requirements to get |
This comment has been minimized.
This comment has been minimized.
Thank you for your reviews. I commited the changes and create a news fragment. |
This comment has been minimized.
This comment has been minimized.
_, import_nodes = decorator.expr.lookup(decorator.expr.name) | ||
|
||
if not import_nodes: | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These last two lines should (hopefully) be easy to cover, also. Get a "functools" that's not from an import statement, and one that's missing (and disable undefined-variable inline, or whatever similar msg)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, those would make pretty low-value tests, so I'm approving now with an eye toward merging later tonight. Thank you!
Sorry about the unhelpful primer message. I opened #8790 for it. |
Thanks for being so responsive @kyoto7250, great first contribution! π |
Type of Changes
Description
Closes #8753