make constants have synthetic root as their parent #2602
Conversation
|
This has in fact uncovered several incorrect tests in pylint, a PR for which I'll make later |
temyurchenko
force-pushed
the
set-constant-roots
branch
from
96c2383
to
3f55e5c
Compare
that fixes a bunch of tests in pylint. We also make the synthetic root a singleton, as opposed to a module built in the astroid manager. That allows us to use it as a default value in Const constructors. Note the changes to the test. Before, we in fact tested that constants don't have a parent. Is that reasonable? In fact, it seems like there are a variety of interpretations and we shouldn't commit to one by explicitly testing for a certain parent or lack thereof.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2602 +/- ##
=======================================
Coverage 93.24% 93.25%
=======================================
Files 93 93
Lines 11049 11070 +21
=======================================
+ Hits 10303 10323 +20
- Misses 746 747 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
temyurchenko
force-pushed
the
set-constant-roots
branch
3 times, most recently
from
96b5d73
to
eab2b06
Compare
temyurchenko
force-pushed
the
set-constant-roots
branch
3 times, most recently
from
bc8109d
to
9848e7d
Compare
temyurchenko
force-pushed
the
set-constant-roots
branch
from
9848e7d
to
17c2f64
Compare
| @@ -248,7 +248,7 @@ def with_metaclass(meta, *bases): | |||
| class metaclass(meta): | |||
| def __new__(cls, name, this_bases, d): | |||
| return meta(name, bases, d) | |||
| return type.__new__(metaclass, 'temporary_class', (), {}) | |||
| return type.__new__(metaclass, 'temporary_class', (), {}) | |||
There was a problem hiding this comment.
Hopefully the crash referred to in the bitbucket issue that has been lost to time didn't depend on this syntax error. 🙏
There was a problem hiding this comment.
From my understanding, the only part that mattered was the «six.with_metaclass» in the line below. The piece of code above could have been either simplified significantly or omitted altogether
There was a problem hiding this comment.
Ugh. Thanks for looking into it.
|
Give me a moment to squash the fixup commit |
jacobtylerwalls
added
the
Maintenance
|
Sure, I'll disable auto-squash, but just so you know my plan was to squash all three commits and use the original message minus the question about the test. Would that have been okay? |
Ah, absolutely fine. |
Hey, can we somehow mark the pylint PR (pylint-dev/pylint#10013) as being necessary to merge with astroid 4.0? Otherwise, there will be confusion about why the tests are failing and there is no obvious indication that they are fixed by that PR. |
|
I have added labels that should help identify it as being necessary :) |
Thank you! |
that fixes a bunch of tests in pylint. We also make the synthetic root a singleton, as opposed to a module built in the astroid manager. That allows us to use it as a default value in Const constructors.
that fixes a bunch of tests in pylint. We also make the synthetic root a singleton, as opposed to a module built in the astroid manager. That allows us to use it as a default value in Const constructors.
that fixes a bunch of tests in pylint.
We also make the synthetic root a singleton, as opposed to a module
built in the astroid manager. That allows us to use it as a default
value in Const constructors.
Note the changes to the test. Before, we in fact tested that constants
don't have a parent. Is that reasonable? In fact, it seems like
there are a variety of interpretations and we shouldn't commit to
one by explicitly testing for a certain parent or lack thereof.
Type of Changes