Skip to content

gh-103498: argparse.ArgumentParser will always throw instead of exit if exit_on_error=False #103519

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

Closed
wants to merge 2 commits into from

Conversation

JustBeYou
Copy link

@JustBeYou JustBeYou commented Apr 13, 2023

Fix for #103498

Summary:

  • moved the decision of if we should raise exception or exit to ArgumentParser.error method based on exit_on_error parameter
  • replaced all raise ArgumentError with self.error() in ArgumentParser
  • added a new named parameter to ArgumentParser.error, namely action=None because raising ArgumentError requires it
  • updated the documentation to reflect the change

Note:

  • adding a new named parameter breaks the API for classes that inherit ArgumentParser; a solution to this would be to use **kwargs instead, but seems kind of dirty to me. suggestions appreciated

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@ghost
Copy link

ghost commented Apr 13, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@JustBeYou JustBeYou force-pushed the gh-103498 branch 2 times, most recently from ec4f0b7 to 1b3c821 Compare April 13, 2023 21:35
@JustBeYou JustBeYou marked this pull request as ready for review April 13, 2023 21:38
@olgarithms
Copy link
Contributor

@sobolevn , do you think this is a good fix? I can add tests to this :)

@JustBeYou
Copy link
Author

Taking a closer look at the problem, I don't see the proposed fix as suitable anymore. I think that to properly solve the problem with exit_on_error, we have to address the inconsistent error handling in ArgumentParser (raising and calling self.error in multiple places). I will share some opinions on the associated issue, maybe we could move forward somehow.

@serhiy-storchaka
Copy link
Member

Thank you for your contribution @JustBeYou. I apologize for the fact that this PR was neglected for a long time. The problem has already been solved in another way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants