-
-
Notifications
You must be signed in to change notification settings - Fork 276
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
Small refactor after 3.12 support #2227
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2227 +/- ##
=======================================
Coverage 92.92% 92.93%
=======================================
Files 95 95
Lines 10918 10920 +2
=======================================
+ Hits 10146 10148 +2
Misses 772 772
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Taking a look |
name: AssignName | None, | ||
type_params: list[TypeVar, ParamSpec, TypeVarTuple], | ||
name: AssignName, | ||
type_params: list[TypeVar | ParamSpec | TypeVarTuple], |
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.
Ouch, thank you, I was just working too quickly.
name: AssignName | ||
type_params: list[TypeVar | ParamSpec | TypeVarTuple] | ||
value: NodeNG |
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'm not totally sure why we're adopting this pattern instead of using the constructor.
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 don't like it as well, but I do think we should at least try and keep one style so that we can then refactor it to a desired style all at once?
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.
No worries. Sometimes it seems like we prefer not to mass-replace, but not a big deal, let's keep moving. π
@@ -2705,16 +2705,19 @@ class ParamSpec(_base_nodes.AssignTypeNode): | |||
<ParamSpec l.1 at 0x7f23b2e4e198> | |||
""" | |||
|
|||
_astroid_fields = ("name",) |
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 missed these because I went back and forth on whether this should be a str
or an AssignName
until I saw the references for the similar issue with MatchStar
and friends pointing to python/cpython#88160.
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.
Let's ship it :-)
Type of Changes
Description
Follow up to #2219.
I didn't get round to reviewing, but had some last nits. This brings the nodes a little bit more in line with the other nodes (I also don't like it, but let's stay consistent).
More importantly, it also fixes an issue with the typing.
list[TypeVar, ParamSpec, ...]
is incorrect as per:https://docs.python.org/3/library/typing.html#annotating-tuples