Fix namedtuple parent #2555
Fix namedtuple parent #2555
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2555 +/- ##
==========================================
+ Coverage 93.22% 93.25% +0.02%
==========================================
Files 93 93
Lines 11048 11044 -4
==========================================
- Hits 10300 10299 -1
+ Misses 748 745 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
temyurchenko
force-pushed
the
fix-namedtuple-parent
branch
from
d3995cc
to
32cf624
Compare
| end_col_offset=None, | ||
| ) | ||
| ] | ||
| tuple_base = util.safe_infer(_extract_single_node("tuple")) |
There was a problem hiding this comment.
Why are you inferring here? Shouldn't this just be the nodes.Name for tuple? Similar to the comment you make about EnumMeta preferably being the Name reference?
There was a problem hiding this comment.
My motivation for using the actual definition instead of a Name was to avoid the situation where the actual tuple is shadowed, and thus the Name refers to something different.
However, looking at it my current implementation doesn't achieve that. A better way to do it would be something like tuple_base = builtin_module["tuple"]. If you think that my worry above is weakly-founded, I can revert to a Name
There was a problem hiding this comment.
(I talk about it in some more detail in the commit message)
There was a problem hiding this comment.
Ah makes sense. I think your idea of doing builtin_module["tuple"] makes the most sense. Let's do that!
There was a problem hiding this comment.
Hmm, actually it appears that tuple_base: nodes.Name = _extract_single_node("tuple") works pretty well. It's not going to be shadowed, because it belongs to an empty module. Should we go with that?
temyurchenko
force-pushed
the
fix-namedtuple-parent
branch
2 times, most recently
from
43e8c5b
to
29d6e84
Compare
There was a problem hiding this comment.
Although I think I'd like @Pierre-Sassoulas to also have a look here
temyurchenko
force-pushed
the
fix-namedtuple-parent
branch
2 times, most recently
from
a147888
to
c4d1be7
Compare
temyurchenko
force-pushed
the
fix-namedtuple-parent
branch
from
c4d1be7
to
534d230
Compare
|
(removed outdated comment, "#set base class=tuple") |
There was a problem hiding this comment.
I feel like those kind of change would benefit from being tested in pylint using the primers. This is not done automatically it would be bad to have surprises after we release a version of astroid. Do you know how to do that @temyurchenko ? (pip install a branch of astroid in editable mode then launch pylint's tests then pylint's primer)
I'll try and report |
temyurchenko
force-pushed
the
fix-namedtuple-parent
branch
from
534d230
to
5b637fa
Compare
temyurchenko
force-pushed
the
fix-namedtuple-parent
branch
from
5b637fa
to
c307783
Compare
@Pierre-Sassoulas, the pylint tests have a fail due to a bug unrelated to this PR (I've submitted a fix here: pylint-dev/pylint#9966). The primer succeeds. So, this one should be safe to merge. |
Pierre-Sassoulas
added
the
pylint-tested
|
Is there anything else to be done with this PR or is it okay to merge, @Pierre-Sassoulas? |
it's a part of the campaign to get rid of non-module roots
temyurchenko
force-pushed
the
fix-namedtuple-parent
branch
from
c307783
to
6a6fc8f
Compare
A part of getting rid of non-Module roots, see #2536