guilatrova / tryceratops

A linter to prevent exception handling antipatterns in Python (limited only for those who like dinosaurs).
MIT License
427 stars 25 forks source link

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

Open Pierre-Sassoulas opened 2 years ago

Pierre-Sassoulas commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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.