argparse exits on error even when exit_on_error=False

[sfermigier]

Bug report

When ArgumentParser encounters certain errors, it calls self.error(...) which exits, even when exit_on_error is set to False.

This prevents catching the exception and writing a custom error message.

import argparse

parser = argparse.ArgumentParser(exit_on_error=False)
parser.add_argument('filename')

try:
    args = parser.parse_args([])
except Exception:
    print("Bad argument, here's what you should do instead...")

Test on Python 3.11.3:

python argparse-bug/repro.py
usage: repro.py [-h] filename
repro.py: error: the following arguments are required: filename

Linked PRs

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

[sunmy2019]

The feature was added at #15362
It looks like the author forgot about this case. A direct modification is acceptable here.

[JustBeYou]

Hi, new contributor here 👋

I written a quick fix which should be generic (instead of guarding all calls to self.error, I added the guard in the method itself). But this way, throwing custom exceptions is not possible. (for example in the PR implementing the feature there was a test expecting a argparse.ArgumentError exception). Is this acceptable or is throwing custom exceptions desired?

If it is desired, we could do the following without breaking the existing API:

1. def error(self, exception_or_message: Union[str, Exception]) - allow passing both messages and exceptions
2. def error(self, message: str, exception_class=None) - allow passing exception class information in a separate argument

Personally, I find the first one the simplest and requires little changes to the code. I would be glad if any maintainer could give an opinion.

[gaogaotiantian]

As far as I can tell, all the self.error() usage is reporting an issue with argument parsing(well, its the argparse library of course). In _parse_known_args there are mixed usage of ArgumentError and self.error() - which is pretty bad.

Doing all the error handling in self.error() seems reasonable, but I don't think we need to change the signature in self.error(). Just raise ArgumentError with the message if self.exit_on_error is False - it's easier for user to catch it anyway.

We also need to replace all the raise ArgumentError with self.error() to keep the code clean.

[gaogaotiantian]

Hmm, I did not realize that ArgumentError actually takes an action, that was my fault. I still think the current implementation is better, but there might be a backward compatibility issue involved. It's clearly stated that the user can override error() function. If you added an extra argument and used it, the user's current code might break. I think we need some opinions from the component owner.

[JustBeYou]

No, I did not reply to the email directly, I just wanted to edit the comment because I noticed the compatibility issue after I written it, but I deleted it instead. 😅

Anyway, I followed your advice, but stumbled upon this action parameter that breaks the API for classes that inherit ArgumentParser. We could avoid this by using **kwargs, but it is not the cleanest way to solve it.

[gaogaotiantian]

Taking at the look at the code, when this feature was intruduced, it was not very thoroughly tested/reviewed. Basically all the self.error() usage need to be vetted - by nature it contradicts this feature. So this feature either has to live in self.error(), or all the current self.error() usage need to be protected by checking this option.