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

optparse: Fix OptionParser types #13339

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

hamdanal
Copy link
Contributor

No description provided.

This comment has been minimized.

Comment on lines -282 to -305
@overload
def parse_args(self, args: None = None, values: Values | None = None) -> tuple[Values, list[str]]: ...
@overload
def parse_args(self, args: Sequence[AnyStr], values: Values | None = None) -> tuple[Values, list[AnyStr]]: ...
Copy link
Contributor Author

@hamdanal hamdanal Dec 29, 2024

Choose a reason for hiding this comment

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

I want to explain the double change of parse_args's args annotation. Consider this snippet:

import optparse

parser = optparse.OptionParser()
parser.add_option("--target-dir", dest="target_dir", help="Target directory")
values, rest = parser.parse_args(["--target-dir", "/tmp"])
print(values, rest)

if you run it, it prints: {'target_dir': '/tmp'} []

  1. args is now annotated as a list instead of a sequence. If you make this change to the above snippet:
    - values, rest = parser.parse_args(["--target-dir", "/tmp"])
    + values, rest = parser.parse_args(("--target-dir", "/tmp"))
    you get: AttributeError: 'tuple' object has no attribute 'pop'
  2. args is now a list of strings, not AnyStr. If you make this change to the above snippet instead:
    - values, rest = parser.parse_args(["--target-dir", "/tmp"])
    + values, rest = parser.parse_args([b"--target-dir", b"/tmp"])
    you get: {'target_dir': None} [b'--target-dir', b'/tmp']
    This is not an error but it is clearly not what you intended as the program failed to parse the arguments in this case.

@hamdanal hamdanal marked this pull request as ready for review December 30, 2024 07:59
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

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.

1 participant