-
-
Notifications
You must be signed in to change notification settings - Fork 308
Fix typing in context.py
#1471
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
Fix typing in context.py
#1471
Conversation
astroid/context.py
Outdated
| self.args = args # Call positional arguments | ||
| if keywords: | ||
| keywords = [(arg.arg, arg.value) for arg in keywords] | ||
| keywords_list = [(arg.arg, arg.value) for arg in keywords] |
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 a fan of putting the type in the variable name: it increase the mental load, you can check the type if you need it and the plural form indicate it's an iterable.
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 changed it to keywords_tuples. Open for any better suggestions.
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 changed it to
keywords_tuples. Open for any better suggestions.
I think that's worse, let's just leave it at keywords. It's assigned to self.keywords after all.
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 meant that keywords was fine :)
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.
Mypy will complain then. keywords is defined as keywords: Optional[List["Keyword"]] = None, on L161. That's why I introduced the name change.
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.
keywords: List[Tuple[str, str]] = [(arg.arg, arg.value) for arg in keywords]Will raise a no-redef warning..
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.
Hmm, maybe keywords_as_list then ? (or keywords_as_tuple)
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.
Hmm, maybe
keywords_as_listthen ?
They are a list already. Maybe keywords_list wasn't that bad after all.
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.
Went with arg_value_pairs 😅 Is that okay?
astroid/context.py
Outdated
| if TYPE_CHECKING: | ||
| from astroid.nodes.node_classes import Keyword, NodeNG | ||
|
|
||
| _InferenceCache = MutableMapping[ |
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.
Any idea why it was MutableMapping instead of Dict?
Do we want to start adding a Type suffix to type aliases?
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.
PR that added it is #1009.
Perhaps adding Type is something to do after we can run mypy successfully? I'm still running into issues with types not actually being correct or missing information. I think we can do a "cleanup" mypy PR after we can get it to pass.
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.
PR that added it is #1009.
Let's use Dict. Usually the return type should be precise whereas the parameter type should be open. Sequence["NodeNG"] is ok though I think. We assign a list converted to a tuple to it.
Perhaps adding
Typeis something to do after we can runmypysuccessfully? I'm still running into issues with types not actually being correct or missing information. I think we can do a "cleanup"mypyPR after we can get it to pass.
👍🏻
for more information, see https://pre-commit.ci
Steps
Description
Had this laying around for quite some time. Mostly moving around some stuff and making sure
keywordsis not reassigned to another type.Type of Changes
Related Issue