search

multimeric / ArgparsePrompt

Wrapper for the built-in Argparse, allowing missing command-line arguments to be filled in by the user via interactive prompts
GNU General Public License v3.0
11 stars 4 forks source link

Catching BaseException and exit(1) in library doesn't allow for error handling #10

Open Jlrine2 opened 3 years ago

Jlrine2 commented 3 years ago

https://github.com/TMiguelT/ArgparsePrompt/blob/aeff7263e2e3aa0a5f25610987808cec6aa322f8/argparse_prompt.py#L94

This library catches a BaseException in Prompt.__call__() this should catch only the specific exceptions that are expected, and raise instead of exit(1) so that I can catch errors and handle them

multimeric commented 3 years ago

I think the idea is that, applying the self.type function to the user's input data could result in any exception. We can't know the possible exceptions beforehand. Possibly this could be tightened to Exception, though.

Jlrine2 commented 3 years ago

Tightening it to Exception would still make me wary, I'd lean toward wanting this to be exception neutral, the self.type function is user code, if it throws the exception should be sent back to the user for handling. If that is the only place an exception is expected, I would lean towards not catching it.

Since it is catching a BaseException over a large block of code, I worry that it will catch unexpected errors and make them a bit harder to find.

Jlrine2 commented 3 years ago

regardless of what is caught, re raising it would be preffered over exit(1)

multimeric commented 3 years ago

Yes, if I were doing anything with the error (like parsing the rest of the arguments), then maybe this would be justified but since I just exit(1) I see no reason to keep the try/except at all. Feel free to put in a PR, otherwise I'll remove it eventually myself.