Skip to content

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 Standard Library Python modules in the Lib/ directory 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 Standard Library Python modules in the Lib/ directory type-bug An unexpected behavior, bug, or error

6 participants