Skip to content

Conversation

@jacobtylerwalls
Copy link
Contributor

@jacobtylerwallsjacobtylerwalls 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
ContributorAuthor

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.

@jacobtylerwallsjacobtylerwalls changed the title bpo-46440: Prevent all exits if ArgumentParser.exit_on_error is Falsebpo-46440: Prevent exits if ArgumentParser.exit_on_error is FalseJan 29, 2022
@jacobtylerwallsjacobtylerwalls changed the title bpo-46440: Prevent exits if ArgumentParser.exit_on_error is Falsebpo-41255: Prevent exits if ArgumentParser.exit_on_error is FalseFeb 5, 2022
@jacobtylerwalls
Copy link
ContributorAuthor

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-TurnerAA-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
@jacobtylerwallsjacobtylerwalls changed the title bpo-41255: Prevent exits if ArgumentParser.exit_on_error is Falsegh-85427: Prevent exits if ArgumentParser.exit_on_error is FalseJun 7, 2022
@jacobtylerwalls
Copy link
ContributorAuthor

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.

@jacobtylerwallsjacobtylerwalls deleted the argparse-exit-on-error branch June 28, 2024 14:50
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting reviewstdlibStandard Library Python modules in the Lib/ directorytype-bugAn 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

@jacobtylerwalls@JelleZijlstra@AA-Turner@sobolevn@serhiy-storchaka@the-knights-who-say-ni