-
-
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
enforce a non-None parent in build_function #2562
enforce a non-None parent in build_function #2562
Conversation
c65d836
to
f3242e8
Compare
Review #2564 first, please |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2562 +/- ##
=======================================
Coverage 93.22% 93.22%
=======================================
Files 93 93
Lines 11048 11048
=======================================
Hits 10300 10300
Misses 748 748
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.
This needs to be rebased and given a new name to the PR, but the localname
change LGTM.
For posterity the localname
stuff was introduced in 4cdfc82
If I understand it correctly, when doing the initial object_build
we try to access getattr(obj, name).__name__
However, this can fail so the code was changed to also have a reference to name
itself as a fallback.
The changes in this PR still allow this pattern so they look good. Please rebase so I can merge it.
Never mind my previous comment. The second commit in this PR was merged in #2557. The first commit was correctly split out to #2564 which I'll approve and merge shortly with the same comment. @temyurchenko Is a commit missing from this PR? Or should it be closed/ |
Hmm, no, that commit was about |
It doesn't exactly fail, it's just that in pypy they do stuff like |
b9d298b
to
8c85892
Compare
8c85892
to
d85731d
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.
Last commit in this PR (after rebase of course) LGTM!
Could you rebase this so I can merge it? :) |
it's a part of the campaign to get rid of non-module roots
d85731d
to
680654c
Compare
Done! |
A part of getting rid of non-Module roots, see #2536
We also remove
add_local_node
to avoid redundancy. Instead we do theattachment to the parent scope in the constructor of
FunctionDef
.We append a node to the body of the frame when it is also the
parent. If it's not a parent, then the node should belong to the
"body" of the parent if it existed. An example is a definition
within an "if", where the parent is the If node, but the frame is
the whole module.
it's a part of the campaign to get rid of non-module roots
Type of Changes
Description
Refs #XXXX
Closes #XXXX