-
-
Notifications
You must be signed in to change notification settings - Fork 273
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
Fix in place properties #2553
Fix in place properties #2553
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2553 +/- ##
==========================================
- Coverage 92.99% 92.99% -0.01%
==========================================
Files 93 93
Lines 11087 11086 -1
==========================================
- Hits 10310 10309 -1
Misses 777 777
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Thanks!
I think this PR shows how much the separate PRs help. Much easier to reason about the changes here in such small scope. Thanks again for doing this!
@@ -14,17 +14,17 @@ | |||
|
|||
class BuiltinsTest(unittest.TestCase): | |||
def test_infer_property(self): | |||
class_with_property = _extract_single_node( | |||
property_assign = _extract_single_node( |
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.
Shall we add a test that property
doesn't end up in the locals
of Something
so we don't regress here?
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.
I've changed the assert below to check that there are no Property
nodes in the locals
.
lineno=node.lineno, | ||
col_offset=node.col_offset, | ||
# ↓ semantically, the definition of this property isn't within |
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.
Could you expand this a little? I think the explanation you gave in the base PR was very useful:
This is not the property being created but the definition of the property
class/function itself. Which doesn't really exist.
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.
I've provided a more detailed explanation. Not sure how coherent it is to others though
This is an example of an in-place property: `bar = property(getter)`. They just create a nameless object, not the one with the name of the getter. Thus, the name was changed to "<property>". Furthermore, the definition of that property is not attached to any scope, as it's again nameless. it's a part of the campaign to get rid of non-module roots
Glad it's helpful! and thank you for your time |
1823ec7
to
1a4cf55
Compare
1a4cf55
to
3dfd5ff
Compare
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.
Awesome, comment is very clear now!
A part of getting rid of non-Module roots, see #2536