Skip to content

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

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

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

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

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

Alternative to #27295

@AA-Turner
Copy link
Member

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
@jacobtylerwalls
Copy link
Contributor Author

jacobtylerwalls commented Apr 15, 2023

@JelleZijlstra Forgive the ping! I just I noticed a duplicate issue (#103498) and PR (#103519) rolled in for this. Do you have a reviewer you might recommend to take a look?

Where we left off was comparing 1.) guarding the calls to self.error() or 2.) altering self.error() itself.

I am honestly not sure which solution is better.

I was somewhat convinced by the comment quoted in #30832 (comment). Plus it's more backwards compatible to leave error() alone. Appreciate any advice, as you may be able!

@JelleZijlstra
Copy link
Member

@sobolevn and @barneygale expressed some interest in argparse issues recently. I don't feel strongly here but I'll try to find some time to read up again and form an opinion.

@sobolevn
Copy link
Member

Also closes: #103498

@sobolevn
Copy link
Member

So, we have at least two PRs that do opposite things.
This PR uses raise ArgumentError: #30832
And the alternative uses self.error() instead: #103519

Now, we need to decide what is better :)

@serhiy-storchaka
Copy link
Member

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

@jacobtylerwalls jacobtylerwalls deleted the argparse-exit-on-error branch June 28, 2024 14:50
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
6 participants