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

Implications of Union in argparse.Action.__call__ parameter types #7559

Open
cebtenzzre opened this issue Mar 27, 2022 · 1 comment
Open

Comments

@cebtenzzre
Copy link
Contributor

def __call__(
self, parser: ArgumentParser, namespace: Namespace, values: str | Sequence[Any] | None, option_string: str | None = ...
) -> None: ...

I'm wondering if the reasoning behind this choice has been discussed before. Return types of library functions generally avoid Union (python/mypy#1693). To me, the parameters of a user-provided callback are similar to the return type of a library function—the annotations reflect objects passed to user code, and the user may have runtime control of their types. It is, however, more removed from the runtime context because the class must be first defined and then referenced.

If I override __call__ with an implementation that will only ever see str:

import argparse

class CSVCallback(argparse.Action):
    def __call__(self, parser, namespace, values, option_string=None):
        setattr(namespace, self.dest, list(values.split(',')))

parser = argparse.ArgumentParser()
parser.add_argument('-o', action=CSVCallback, default=[])

args = parser.parse_args()

pyright complains, based on what appears to be a correct interpretation of the stub:

  repr.py:5:51 - error: Cannot access member "split" for type "Sequence[Any]"
    Member "split" is unknown (reportGeneralTypeIssues)
  repr.py:5:51 - error: "split" is not a known member of "None" (reportOptionalMemberAccess)

Would the appropriate solution be to add assert isinstance(values, str) at the beginning of every callback? Or is this actually abuse of action=..., which should be replaced with type=..., like this?

def csv_callback(value: str) -> list[str]:
    return list(value.split(','))

parser.add_argument('-o', type=csv_callback, default=[])

Since these examples are based on 10-year-old optparse code that was ported 1:1 to argparse 2 years ago, I'm not considering reimplementing them like that—they aren't broken, and the less trivial examples reference parser.error and the previous value of self.dest. But I'd rather avoid the asserts, so for new code I would probably prefer that pattern.

@cebtenzzre cebtenzzre changed the title Implications of Union in argparse.Action.\_\_call\_\_ parameter types Implications of Union in argparse.Action.__call__ parameter types Mar 27, 2022
@srittau
Copy link
Collaborator

srittau commented Apr 16, 2022

I believe the best solution is to introduce a _ActionValue (or similar) type alias to Any and use that as the values type. A comment could then be added about the possible types. PR welcome.

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

No branches or pull requests

2 participants