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

Possible overlap between TRY400 and G201 #4603

Closed
ericbn opened this issue May 23, 2023 · 2 comments · Fixed by #4797
Closed

Possible overlap between TRY400 and G201 #4603

ericbn opened this issue May 23, 2023 · 2 comments · Fixed by #4797
Assignees
Labels
question Asking for support or clarification

Comments

@ericbn
Copy link
Contributor

ericbn commented May 23, 2023

test.py

Minimal code snipped that reproduces the bug:

import logging

logger = logging.getLogger()
try:
    pass
except Exception:
    logger.error("Error", exc_info=True)

Ruff output

The command I've invoked: ruff --isolated --select G,TRY test.py

test.py:7:5: TRY400 Use `logging.exception` instead of `logging.error`
test.py:7:12: G201 Logging `.exception(...)` should be used instead of `.error(..., exc_info=True)`
Found 2 errors.

After changing the test.py file to:

-    logger.error("Error", exc_info=True)
+    logger.error("Error")

I get only:

test.py:7:5: TRY400 Use `logging.exception` instead of `logging.error`
Found 1 error.

Maybe the broader TRY400 rule is enough, in favor of G201. Not sure what's the rationale for overlapping/duplicate rules in ruff.

ruff --version

ruff 0.0.269
@zanieb
Copy link
Member

zanieb commented May 23, 2023

Hm logger.exception has a different meaning than logger.errorlogger.error does not include a traceback unless exc_info is set but logger.exception does. I think these rules cover different cases.

  • G201: If you're going to log a traceback, use .exception instead of .error — this could be an automatic fix because the meaning is retained
  • TRY400: If you're going to log an error, always log a traceback — this would only be a suggested fix because it changes behavior

@charliermarsh
Copy link
Member

Thank you for the extremely clear issue, appreciated as always. I think these are slightly different, but what we could do is change TRY400 to avoid flagging logger.error calls with exc_info=True, since those will include a traceback anyway as per the intent of the rule. (Then, suggesting the use of logging.exception over exc_info=True would be covered separately by G201.) What do you think, @madkinsz?

@charliermarsh charliermarsh added the question Asking for support or clarification label May 24, 2023
@charliermarsh charliermarsh self-assigned this Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Asking for support or clarification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants