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

gh-85427: Prevent exits if ArgumentParser.exit_on_error is False #30832

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

Conversation

jacobtylerwalls
Copy link
Contributor

@jacobtylerwalls jacobtylerwalls commented Jan 23, 2022

bpo-41255

The exit_on_error docs read:

   * exit_on_error_ - Determines whether or not ArgumentParser exits with
     error info when an error occurs. (default: ``True``)

From this, I agree with the reporter that all exit paths should raise an exception rather than exit.

https://bugs.python.org/issue46440

Fixes #85427

@JelleZijlstra
Copy link
Member

JelleZijlstra commented Jan 24, 2022

Looking inside _parse_known_args, I see some error paths doing raise ArgumentError and others calling self.error, without any apparent pattern. If they all did raise ArgumentError, that would fix this bug.

@JelleZijlstra
Copy link
Member

JelleZijlstra commented Jan 24, 2022

There are other calls to self.error elsewhere in the class that may be invoked from _parse_known_args, so this still leaves some paths where it may exit. In that sense, your original solution was more robust. I am honestly not sure which solution is better.

@jacobtylerwalls
Copy link
Contributor Author

jacobtylerwalls commented Jan 29, 2022

I found a duplicate with some more discussion: bpo-41255.

From the penultimate comment:

I'm not sure I like the idea of changing the exit or error methods since they have a a clear purpose and don't need to be repurposed to also include error handling. It seems to me that adding checks to self.exit_on_error in _parse_known_args to handle the missing required arguments and in parse_args to handle unknown arguments is probably a quick and clean solution.

@jacobtylerwalls jacobtylerwalls changed the title bpo-46440: Prevent all exits if ArgumentParser.exit_on_error is False bpo-46440: Prevent exits if ArgumentParser.exit_on_error is False Jan 29, 2022
@jacobtylerwalls jacobtylerwalls changed the title bpo-46440: Prevent exits if ArgumentParser.exit_on_error is False bpo-41255: Prevent exits if ArgumentParser.exit_on_error is False Feb 5, 2022
@jacobtylerwalls
Copy link
Contributor Author

jacobtylerwalls commented Feb 5, 2022

Alternative to #27295

@AA-Turner
Copy link
Member

AA-Turner commented Jun 7, 2022

Backref to #85427 (@jacobtylerwalls please could you update the title to start with gh-85427?).

A

@AA-Turner AA-Turner added type-bug An unexpected behavior, bug, or error awaiting review stdlib Python modules in the Lib dir and removed CLA signed labels Jun 7, 2022
@jacobtylerwalls jacobtylerwalls changed the title bpo-41255: Prevent exits if ArgumentParser.exit_on_error is False gh-85427: Prevent exits if ArgumentParser.exit_on_error is False Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Argparse.parse_args exits on unrecognized option with exit_on_error=False
4 participants