Skip to content
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

Drop some literal types from argparse (add_argument) #7614

Merged
merged 2 commits into from
Apr 13, 2022

Conversation

JukkaL
Copy link
Contributor

@JukkaL JukkaL commented Apr 12, 2022

These were introduced in #7329 and they cause false positives
in code that used to be accepted before. There was a false
positive in https://github.com/pycqa/pylint (encountered in
python/mypy#12321) and I also saw
false positives in an internal codebase.

I suggest not using literal types here, since they can be kind of
awkward to annotate in callers that don't pass a literal string.
Requiring callers to write annotations like
Literal["?", "*", "+", "...", "A...", "==SUPPRESS=="] is not
very user friendly.

These were introduced in python#7329 and they cause false positives
in code that used to be accepted before. There was a false
positive in https://github.com/pycqa/pylint (encountered in
python/mypy#12321) and I also saw
false positives in an internal codebase.

I suggest not using literal types here, since they can be kind of
awkward to annotate in callers that don't pass a literal string.
Rrequiring callers to write annotations like
`Literal["?", "*", "+", "...", "A...", "==SUPPRESS=="]` is not
very user friendly.
@srittau
Copy link
Collaborator

srittau commented Apr 12, 2022

I'm not terribly happy about reverting these changes. Catching typos in "enum"-like strings is a perfect use case for type checking. But the lack of usability for these types of annotations remains a concern that needs to be discussed.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

I have similar thoughts to @srittau. Downgrading the precision of the stubs is something I'm never particularly happy about. But I can definitely see the usability concern here, so I guess I'm +0 on this change.

Pinging @JelleZijlstra and @Akuli, who were involved in reviewing the original PR.

@JukkaL
Copy link
Contributor Author

JukkaL commented Apr 12, 2022

But the lack of usability for these types of annotations remains a concern that needs to be discussed.

I think that this is the key issue. The types aren't available anywhere where they can be easily found. Even if we add a type alias in the stub, they won't be easily discoverable and can't be imported at runtime. I agree that it would be nice if we could check these, but I don't see a way to do this without a bad user experience.

Also, the benefit of type checking these seems a bit limited since the error would be caught immediately after running the program. argparse does pretty decent runtime checking, and argument parsing is usually performed always at the beginning of execution. Here's an example traceback:

Traceback (most recent call last):
  File "/Users/jukka/.../t.py", line 4, in <module>
    p.add_argument('--py', action='xyz')
  File "/Library/Frameworks/Python.framework/Versions/3.10/lib/python3.10/argparse.py", line 1417, in add_argument
    raise ValueError('unknown action "%s"' % (action_class,))
ValueError: unknown action "xyz"

@srittau
Copy link
Collaborator

srittau commented Apr 12, 2022

Let's at least define aliases to str (_ActionType and _NArgs?) for the former literals and keep the literal values as comments.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

pylint (https://github.com/pycqa/pylint)
- pylint/config/arguments_manager.py:134: error: Argument "action" to "add_argument" of "_ActionsContainer" has incompatible type "str"; expected "Union[Type[Action], Literal['store', 'store_const', 'store_true', 'store_false', 'append', 'append_const', 'count', 'help', 'version', 'extend']]"  [arg-type]
- pylint/config/arguments_manager.py:179: error: Argument "action" to "add_argument" of "_ActionsContainer" has incompatible type "str"; expected "Union[Type[Action], Literal['store', 'store_const', 'store_true', 'store_false', 'append', 'append_const', 'count', 'help', 'version', 'extend']]"  [arg-type]

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.

Thanks!

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