-
-
Notifications
You must be signed in to change notification settings - Fork 294
Mandatory fields for Assign #2061
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
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2061 +/- ##
==========================================
- Coverage 92.78% 92.78% -0.01%
==========================================
Files 94 94
Lines 11046 11043 -3
==========================================
- Hits 10249 10246 -3
Misses 797 797
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.
Shouldn't we change the constructor for _base_nodes.AssignTypeNode, _base_nodes.Statement
too ?
@Pierre-Sassoulas Are we okay with merging breaking changes into |
Let's consider that if it's not breaking pylint then it's not a change that require a major version bump even if technically it's a breaking change. Especially if it's typing, as we never advertised astroid as typed. |
It looks like those don't have their own constructors. They all just delegate to |
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'd rather have the overridden constructor requiring 'targets' and remove the post init function altogether. But for that to work we would need to refactor the tree rebuilder. I suppose if we instantiate the leafs first and then recurse instead of what we're currently doing it's possible (?). What do you think @DanielNoord ?
I mean, it's worth a shot but it would be a massive rewrite. I would be okay with reviewing a complete rewrite and contributing but don't have the time to do to brunt of the work.. |
Getting rid of As for the two-stage initialization, it's one of many Chesterton fences in Astroid. Why is it like that? There may have been a good reason long ago, but that reason may be lost. One good reason to keep it that way is that nodes have generic information (line numbers, etc) and type-specific information (arguments, values, etc). The two-stage initialization allows for that information to be set up separately. This way, the generic information only needs to be handled in one place. (The current arrangement gets the worst of both worlds, since the generic information is declared in every single node type, only to be passed along to |
parent=parent, | ||
) | ||
_astroid_fields = ("targets", "value") | ||
_other_other_fields = ("type_annotation",) | ||
|
||
def postinit( |
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.
Line 808 in 345b91c
newnode.postinit( |
Could you add asserts here that the typing we add here is correct?
And could you add a changelog entry? For the rest this LGTM!
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 see myself doing a massive rewrite (when there are a lot of massive rewrite we could do) so let's merge, it's an upgrade in the current state of the code.
Agreed! But let's at least make sure that all the callsites of these functions, so everywhere we do |
What about the rest of the node types? Many of the rest need a similar fix. (Some require more involved changes.) Should that be done here, or separate PRs? One PR per node type, or all at once? |
Let's do separate PRs! I think I can check these fairly quickly but focusing on one node at a time makes it much easier. You can tag me for review (although please don't open 10 at once π ) |
9f5b43d
to
91bd352
Compare
Type of Changes
Description
This PR re-types the required fields of the
Assign
node so that they are non-optional, as described in #2060.Two existing type errors are thereby fixed, as reported by Mypy. Here is one of them:
If
Assign.value
can beNone
, then this could fail. But if the field is required, there is no problem.