Add overloads for argparse functions that take a namespace to return a typevar bound to the input namespace #10307
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…a typevar bound to the input namespace
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
srittau
requested changes
Jun 12, 2023
There was a problem hiding this comment.
Thanks! A few issues below.
We need to add an additional overload per method, similar to how parse_args() has it:
@overload
def parse_args(self, *, namespace: _N) -> _N: ...This handles the case when someone calls parse_args(namespace=...).
Also, Namespace | None as annotation for the second argument of the second overload is not wrong per-se, but it's redundant with the first overload, so just using None (like parse_args() does it) is a bit clearer.
Finally, namespace is not required to derive from Namespace. In fact, the example in the Python docs uses such a class. The previous type var without the bound was correct.
|
@srittau I have addressed those changes. Thank you for the corrections and suggestions. |
This comment has been minimized.
This comment has been minimized.
srittau
reviewed
Jun 12, 2023
stdlib/argparse.pyi
Outdated
| self, args: Sequence[str] | None = None, namespace: Namespace | None = None | ||
| ) -> tuple[Namespace, list[str]]: ... | ||
| @overload | ||
| def parse_known_args(self, args: Sequence[str] | None = None, namespace: None = None) -> tuple[Namespace, list[str]]: ... # type: ignore[misc] |
There was a problem hiding this comment.
I'm sorry for the churn, but could you try re-adding Namespace here?
Suggested change
| def parse_known_args(self, args: Sequence[str] | None = None, namespace: None = None) -> tuple[Namespace, list[str]]: ... # type: ignore[misc] | |
| def parse_known_args(self, args: Sequence[str] | None = None, namespace: Namespace | None = None) -> tuple[Namespace, list[str]]: ... # type: ignore[misc] |
I know I asked you previously to remove it, but it seems that mypy has its problems with that in cases like this: https://github.com/pytest-dev/pytest/blob/2d824329ebf5455aaf4c6f8aa29afff401abe315/src/_pytest/config/argparsing.py#LL439C37-L439C53
I just want to see if that works better. (If it does, we should add Namespace to all the overloaded functions.)
There was a problem hiding this comment.
Just pushed this change.
There was a problem hiding this comment.
Looks like the pytest errors in primer have been resolved.
|
(The new primer hits for materialize can be ignored. They need to update their type annotations to match the new annotations.) |
|
Diff from mypy_primer, showing the effect of this PR on open source code: materialize (https://github.com/MaterializeInc/materialize)
+ misc/python/materialize/mzcompose/__init__.py:904: error: Signature of "parse_known_args" incompatible with supertype "ArgumentParser" [override]
+ misc/python/materialize/mzcompose/__init__.py:904: note: Superclass:
+ misc/python/materialize/mzcompose/__init__.py:904: note: @overload
+ misc/python/materialize/mzcompose/__init__.py:904: note: def parse_known_args(self, args: Optional[Sequence[str]] = ..., namespace: Optional[Namespace] = ...) -> Tuple[Namespace, List[str]]
+ misc/python/materialize/mzcompose/__init__.py:904: note: @overload
+ misc/python/materialize/mzcompose/__init__.py:904: note: def [_N] parse_known_args(self, args: Optional[Sequence[str]], namespace: _N) -> Tuple[_N, List[str]]
+ misc/python/materialize/mzcompose/__init__.py:904: note: @overload
+ misc/python/materialize/mzcompose/__init__.py:904: note: def [_N] parse_known_args(self, *, namespace: _N) -> Tuple[_N, List[str]]
+ misc/python/materialize/mzcompose/__init__.py:904: note: Subclass:
+ misc/python/materialize/mzcompose/__init__.py:904: note: def parse_known_args(self, args: Optional[Sequence[str]] = ..., namespace: Optional[Namespace] = ...) -> Tuple[Namespace, List[str]]
+ misc/python/materialize/cli/mzcompose.py:670: error: Signature of "parse_known_args" incompatible with supertype "ArgumentParser" [override]
+ misc/python/materialize/cli/mzcompose.py:670: note: Superclass:
+ misc/python/materialize/cli/mzcompose.py:670: note: @overload
+ misc/python/materialize/cli/mzcompose.py:670: note: def parse_known_args(self, args: Optional[Sequence[str]] = ..., namespace: Optional[Namespace] = ...) -> Tuple[Namespace, List[str]]
+ misc/python/materialize/cli/mzcompose.py:670: note: @overload
+ misc/python/materialize/cli/mzcompose.py:670: note: def [_N] parse_known_args(self, args: Optional[Sequence[str]], namespace: _N) -> Tuple[_N, List[str]]
+ misc/python/materialize/cli/mzcompose.py:670: note: @overload
+ misc/python/materialize/cli/mzcompose.py:670: note: def [_N] parse_known_args(self, *, namespace: _N) -> Tuple[_N, List[str]]
+ misc/python/materialize/cli/mzcompose.py:670: note: Subclass:
+ misc/python/materialize/cli/mzcompose.py:670: note: def parse_known_args(self, args: Optional[Sequence[str]] = ..., namespace: Optional[Namespace] = ...) -> Tuple[Namespace, List[str]]
+ misc/python/materialize/cli/mzcompose.py:681: error: Signature of "parse_known_args" incompatible with supertype "ArgumentParser" [override]
+ misc/python/materialize/cli/mzcompose.py:681: note: Superclass:
+ misc/python/materialize/cli/mzcompose.py:681: note: @overload
+ misc/python/materialize/cli/mzcompose.py:681: note: def parse_known_args(self, args: Optional[Sequence[str]] = ..., namespace: Optional[Namespace] = ...) -> Tuple[Namespace, List[str]]
+ misc/python/materialize/cli/mzcompose.py:681: note: @overload
+ misc/python/materialize/cli/mzcompose.py:681: note: def [_N] parse_known_args(self, args: Optional[Sequence[str]], namespace: _N) -> Tuple[_N, List[str]]
+ misc/python/materialize/cli/mzcompose.py:681: note: @overload
+ misc/python/materialize/cli/mzcompose.py:681: note: def [_N] parse_known_args(self, *, namespace: _N) -> Tuple[_N, List[str]]
+ misc/python/materialize/cli/mzcompose.py:681: note: Subclass:
+ misc/python/materialize/cli/mzcompose.py:681: note: def parse_known_args(self, args: Optional[Sequence[str]] = ..., namespace: Optional[Namespace] = ...) -> Tuple[Namespace, List[str]]
|
|
#8553 was similar. I felt hesistant to merge because of how subclassing becomes more complicated. |
Jakob-Stadler
added a commit
to Jakob-Stadler/typeshed
that referenced
this pull request
Jun 30, 2023
The previous commit [1] explicitly added `Namespace` to the first overload, which causes a false-postive when the used `namespace=` argument is a subclass of `Namespace`.
Example code, flagged both in pyright and mypy (with most recent typeshed):
```python
import argparse
import sys
# Explicit subclass of Namepsace
# not strictly necessary, but it conveys intent better than a standalone class
class ExampleNamespace(argparse.Namespace):
example_argument: str
parser = argparse.ArgumentParser()
parser.add_argument('example_argument')
ns = parser.parse_args(sys.argv, ExampleNamespace())
reveal_type(ns) # Revealed type is "Namespace"
# error: Incompatible types in assignment (expression has type "Namespace", variable has type "ExampleNamespace") [assignment]
ens: ExampleNamespace = parser.parse_args(sys.argv, ExampleNamespace())
reveal_type(ens) # Revealed type is "ExampleNamespace"
```
[1] python#10307
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the
argparsestubs,ArgumentParser.parse_argshas overloads that allow subclasses ofNamespaceas inputs that return that same type (using aTypeVar). At runtime, the methodsparse_known_args,parse_intermixed_args, andparse_known_intermixed_argsshadow this same behavior, but the types do not reflect this. In a codebase I help maintain, we cannot easily type check our CLI becauseparse_known_argsis currently typed to return aNamespaceinstead of subclasses.This PR adds namespace typevar overloads to the aforementioned methods, mirroring that of
parse_args.