Skip to content

Add private members to argparse.pyi. #1937

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 14 commits into from
Apr 11, 2018
Merged

Add private members to argparse.pyi. #1937

merged 14 commits into from
Apr 11, 2018

Conversation

rchen152
Copy link
Collaborator

@rchen152 rchen152 commented Mar 1, 2018

These attributes are sometimes used despite being private, especially the Action subclasses.

Based on https://github.com/python/cpython/blob/master/Lib/argparse.py.

@matthiaskramm
Copy link
Contributor

Is there some issue with mypy travis right now? We're seeing:

FAILURE  #5 check package mypy
mypy/main.py:235: note: unused 'type: ignore' comment
mypy/main.py:240: note: unused 'type: ignore' comment

@JelleZijlstra
Copy link
Member

Those lines are like arg = parser.add_argument(flag, # type: ignore # incorrect stub for add_argument. It looks like this PR fixes the incorrect stubs.

We should merge this (after it's reviewed), then immediately make a PR to mypy syncing typeshed and removing the type: ignores.

@rchen152
Copy link
Collaborator Author

ping
Could I get a review of this soon?

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Most of these changes look correct, although I left a couple of minor comments on specific points.

Adding this many private methods makes me uneasy. It will be hard to get all the version checks right and to keep things up to date. However, in #1902 we recently agreed to let this sort of type information in, provided that we use # undocumented comments where appropriate. Could you add those?

)
import sys

_T = TypeVar('_T')
_ActionVar = TypeVar('_ActionVar', bound='Action')
Copy link
Member

Choose a reason for hiding this comment

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

Usually we suffix typevars with T (e.g., _ActionT).

def _registry_get(self, registry_name: _Text, value: Any, default: Any = ...) -> Any: ...
def set_defaults(self, **kwargs: Any) -> None: ...
def get_default(self, dest: _Text) -> Any: ...
def add_argument(self, *args: Any, **kwargs: Any) -> Action: ...
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the more specific arguments to this function in the previous stub?

Copy link
Collaborator Author

@rchen152 rchen152 Mar 21, 2018

Choose a reason for hiding this comment

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

Those were incorrect. I'm actually not entirely sure where they came from, given that in argparse.py, add_argument is defined using *args and **kwargs.

Copy link
Member

Choose a reason for hiding this comment

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

The Python-visible signature is *args, **kwargs, but obviously in practice this method doesn't take any argument. Since it's probably the most frequently called part of argparse's interface, I think it's important that we give more specific types where it's at all possible. The documentation (https://docs.python.org/3.7/library/argparse.html#argparse.ArgumentParser.add_argument) also gives more specific arguments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I see! I think I was confused by the docstring for add_argument, which looks pretty different from the documentation. Done.

def __init__(self, **kwargs: Any) -> None: ...
def __getattr__(self, name: _Text) -> Any: ...
def __setattr__(self, name: _Text, value: Any) -> None: ...
def __contains__(self, key: str) -> bool: ...

class FileType:
_mode: _Text
_bufsize: int
_encoding: Optional[_Text]
Copy link
Member

Choose a reason for hiding this comment

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

Do these exist in Python 2? The construction signatures below suggests they don't.

@JelleZijlstra JelleZijlstra mentioned this pull request Mar 18, 2018
41 tasks
@rchen152
Copy link
Collaborator Author

Addressed all the comments.

@JelleZijlstra JelleZijlstra merged commit e2cfc4b into python:master Apr 11, 2018
@rchen152 rchen152 deleted the argparse branch June 15, 2018 20:38
yedpodtrzitko pushed a commit to yedpodtrzitko/typeshed that referenced this pull request Jan 23, 2019
* Add _AttributeHolder and _ActionsContainer classes to argparse.

* Add Action subclasses to argparse.

* Add _UNRECOGNIZED_ARGS_ATTR, _ensure_value, _get_action_name to argparse.

* Fill in remaining _ActionsContainer attributes.

* Fill in missing argparse.ArgumentParser attributes.

* Fill in missing argparse.HelpFormatter attributes.

* Fill in remaining missing attributes on argparse classes.

* Rename TypeVar _ActionVar to _ActionT

* Add a version check for FileType attributes

* Add '# undocumented' where appropriate

* Add more # undocumented comments

* Make arguments to _ActionsContainer.add_argument more precise.
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.

3 participants