Skip to content
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

False positive use 'exception' instead of 'error' when raisign a parser error with argparse #35

Open
Pierre-Sassoulas opened this issue Oct 29, 2021 · 5 comments

Comments

@Pierre-Sassoulas
Copy link

Pierre-Sassoulas commented Oct 29, 2021

In the following code the parser is mistaken for a logger and using parser.exception is suggested :

import argparse

parser = argparse.ArgumentParser()
parser.add_argument(
    "config_folder", help="Path to the project configuration folder"
)
args = parser.parse_args()
try:
    int(args.config_folder) / 0
except:
    parser.error(
        f"Your configuration folder can't be divided by zero, use something else !"
    )
@guilatrova
Copy link
Owner

Hi @Pierre-Sassoulas , thanks for sharing! I got 2 questions for you:

  1. Do you think it would make sense to just add a # noqa: TC400?

  2. I wonder if you have any ideas on how we can smartly define whether it's a "logger" or not.
    Because people can create many variables, e.g. logger, companylog, mylogger, and it's hard to identify where it comes from.

If you don't know about 2, don't worry. I'm mostly brainstorming here.

@Pierre-Sassoulas
Copy link
Author

Pierre-Sassoulas commented Oct 29, 2021

Full disclosure I'm biased because I'm maintaining it, but astroid permits to infer values:

>>> import astroid
>>> a = astroid.extract_node('''
... import argparse
... 
... parser = argparse.ArgumentParser()
... parser.add_argument(
...     "config_folder", help="Path to the project configuration folder"
... )
... args = parser.parse_args()
... try:
...     int(args.config_folder) / 0
... except:
...     parser.error(
...         f"Your configuration folder can't be divided by zero, use something else !"
...     )
... ''')
>>> list(a.handlers[0].body[0].value.func.infer())[0]
<BoundMethod error of argparse.ArgumentParser at 0x140506382647696>

@woutervh
Copy link

woutervh commented Mar 7, 2022

I disagree that TC400 is an antipattern.
In some cases you don't need the full traceback.

The python-docs only say:
"This method should only be called from an exception handler."
Logical, because there exists no traceback when no exception occurred.

It does not say: "In an exception handler only this method must be called."

I avoid log.exception because it is not clear what is does.
I standardize my code with:

  log.error(f"Error ....", exc_info=True|False)

Then you can make logging if the traceback even conditional if needed.

@guilatrova
Copy link
Owner

guilatrova commented Mar 19, 2022

I disagree that TC400 is an antipattern.

@woutervh I welcome you to create a new issue if you're interested in discussing this. Anyway, feel free to go ahead and disable it in your project.

@edvardm
Copy link

edvardm commented Jun 6, 2023

Fully agree with @woutervh

Using logger.exception can be used in exception handler, but it doesn't need to. I think this is misinterpreting what Python docs say.

For me there's are two drastically different kinds of exceptions. First one is catching Exception which means I don't know what went wrong, and there I often do want to use logger.exception to see automatically the stack trace and where the error originated. With better_exceptions library I even see offending values.

Expected exceptions that are more specific and derived from Exception are totally different. These I expect and usually know what caused the error, so I don't want a stacktrace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants