guilatrova / tryceratops

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

fix: allow f-string to trigger TRY003 #75

Closed LSanselme closed 1 week ago

LSanselme commented 2 months ago

Hello, and thank you for this package!

I stumble across the issue https://github.com/guilatrova/tryceratops/issues/31 and may have a potential fix. The problem was false negative for rule TRY003, "Avoid specifying long messages outside the exception class", the root cause being the use of f-string in such messages.

The solution simply consists of checking whether the string is an f-string using ast.JoinedStr.

However, this fix is not perfect, as illustrated by the following example:

    elif a == 5:
        raise CustomException(f"code_{a}")  # This should be acceptable

This code triggers TRY003 with the fix, but it should be acceptable since there are no whitespaces, like in the existing test

    elif a == 3:
        raise CustomException("its_code_not_message")  # This is acceptable

The challenge is that, to my knowledge, there is no way to determine the content of an f-string during parsing without evaluating it first. It is probably beyond the scope of static analysis.

I think it is related to why Google discourages the use of f-string for logging in their styleguide. If f-strings are considered a bad practice for exceptions as well,, it might be worth adding a new violation rule in Tryceratops specifically for them.

Please let me know if you have any feedback or suggestions for improving this fix.

LSanselme commented 2 months ago

For the record, ruff's implementation of TRY003 does produce fewer false positive than this fix. Their solution involves iterating over the str elements of the f-string and trigger a violation only if any contains a whitespace:

a="0"
raise CustomException(f"code_{a}")  # This is accepted by ruff, as it should be
raise CustomException(f"message {a}")  # This is not accepted by ruff, as it should be

For the reason mentioned previously (ruff being a static linter as well), it can still produce false negative in the following case:

a = " 0"  # there is a space here
raise CustomException(f"message{a}")  # This is accepted by ruff, but it shouldn't be

[edit 2024-08-22]: I finally implemented it in 67786fb

guilatrova commented 1 week ago

Tested locally and it's fine. Thank you :)