Skip to content

correct parse_args namespace attribute #2566

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

Merged
merged 9 commits into from
Oct 28, 2018
Merged

correct parse_args namespace attribute #2566

merged 9 commits into from
Oct 28, 2018

Conversation

PrajwalM2212
Copy link
Contributor

fixes #2366

This PR allows parse_args namespace arg to accept any object and also to return any object

@@ -122,7 +122,7 @@ class ArgumentParser(_AttributeHolder, _ActionsContainer):
conflict_handler: _Text = ...,
add_help: bool = ...) -> None: ...
def parse_args(self, args: Optional[Sequence[_Text]] = ...,
namespace: Optional[Namespace] = ...) -> Namespace: ...
namespace: Optional[object] = ...) -> object: ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the right solution. There are basically three cases that can be distinguished using @overload:

  1. No namespace argument given -> return Namespace
  2. namespace is None -> return Namespace
  3. namespace is any other value: return the same type. This can be achieved using a TypeVar.

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks much better. Two issues, though.

_N = TypeVar('_N')
@overload
def parse_args(self, args: Optional[Sequence[_Text]] = ...,
namespace: Type[_N] = ...) -> _N: ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to remove the default from the namespace parameter here. Otherwise type checkers get confused when calling parse_args([]), because both overloads match.

Also, the namespace passed in is returned unchanged, not used as a constructor. So the annotation for namespace should be just _N, not Type[_N].

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addendum: If you remove the default here, you should not need the third overload you just added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I remove the default by setting namespace: _N , pycharm high-lightens it as non-default parameter follows default parameter. Should I ignore it ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my bad. Try these two lines instead of the one above:

@overload
def parse_args(self, args: Optional[Sequence[_Text]], namespace: Type[_N]) -> _N: ...
@overload
def parse_args(self, *, namespace: Type[_N]) -> _N: ...

@@ -121,8 +121,16 @@ class ArgumentParser(_AttributeHolder, _ActionsContainer):
argument_default: Optional[_Text] = ...,
conflict_handler: _Text = ...,
add_help: bool = ...) -> None: ...

@overload
def parse_args(self, args: Optional[Sequence[_Text]] = ...,
namespace: Optional[Namespace] = ...) -> Namespace: ...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this overload shouldn't have the namespace argument. If namespace is given, the second overload suffices.

@PrajwalM2212
Copy link
Contributor Author

Closing ! Will reopen this when I understand more about how overload works in mypy

@PrajwalM2212
Copy link
Contributor Author

@srittau @JelleZijlstra Thanks for your generous help. I will reopen this when I understand more about how overload works in mypy.

@srittau
Copy link
Collaborator

srittau commented Oct 28, 2018

@PrajwalM2212 Thank you for your effort to fix those small bugs, I really appreciate it! I think you were nearly there with this PR. Could you try this:

    _N = TypeVar('_N')
    @overload
    def parse_args(self, args: Optional[Sequence[_Text]] = ...) -> Namespace: ...
    @overload
    def parse_args(self, args: Optional[Sequence[_Text]], namespace: _N) -> _N: ...
    @overload
    def parse_args(self, *, namespace: _N) -> _N: ...

This changes two things compared to your last version:

  • It moved the typevar before the first overload. It seems type checkers don't like it if it's between the overloads. You could also move the typevar to the top of the file, which we usually do
  • Remove Type around the namespace argument (see my previous comment).

@PrajwalM2212 PrajwalM2212 reopened this Oct 28, 2018
@srittau
Copy link
Collaborator

srittau commented Oct 28, 2018

I am sorry that it failed again. It seems that pytype does not like typevars inside classes. I didn't know that. So just move it to the start of the file and then it should finally pass! Thank you for your persistence.

@srittau srittau merged commit 60000d0 into python:master Oct 28, 2018
yedpodtrzitko pushed a commit to yedpodtrzitko/typeshed that referenced this pull request Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

argparse.parse_args should have a generic return type
3 participants