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

[flake8-logging] Implement check for logging.exception() outside of exception handler (LOG004) #14245

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JCWasmx86
Copy link

@JCWasmx86 JCWasmx86 commented Nov 10, 2024

Relates to #7248

Summary

This implements partially LOG004 of flake8-logging:

logging.exception('') # Should be replaced by logging.error
try:
  logging.exception('') # Should be replaced by logging.error
except:
  logging.exception('') # That's fine
  def handle():
    logging.exception('') # Should be replaced, as the function can be called from everywhere

What's currently not implemented:

logger = logging.getLogger("foo")
logger.exception("abc")

That's because I'm unsure what's the best way to implement it:

  • Like the plugin is doing: Walk the ast and find all identifiers that look like $ID = logging.getLogger(...) or $ID = logging.Logger(...)
  • (Not sure how possible it is with current ruff): Check whether the variable has the type Logger
  • Heuristics (Variable named like LOG/LOGGER/loggger/etc. and called on a method called exception)

The docs are nearly copied from the flake8-logging readme (Not sure, if that's allowed/how to properly attribute)

Test Plan

With a manual test file (Parts of it included here), inspired by the tests in the flake8-logging repo.

Copy link
Contributor

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+3 -0 violations, +0 -0 fixes in 2 projects; 52 projects unchanged)

pandas-dev/pandas (+0 -0 violations, +0 -0 fixes)


zulip/zulip (+3 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --no-preview --select ALL

+ zerver/worker/base.py:265:17: LOG004 [*] Use of `logging.exception` outside exception handler
+ zerver/worker/base.py:267:17: LOG004 [*] Use of `logging.exception` outside exception handler
+ zerver/worker/email_senders.py:39:17: LOG004 [*] Use of `logging.exception` outside exception handler

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
LOG004 3 3 0 0 0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+3 -0 violations, +0 -0 fixes in 1 projects; 53 projects unchanged)

zulip/zulip (+3 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ zerver/worker/base.py:265:17: LOG004 [*] Use of `logging.exception` outside exception handler
+ zerver/worker/base.py:267:17: LOG004 [*] Use of `logging.exception` outside exception handler
+ zerver/worker/email_senders.py:39:17: LOG004 [*] Use of `logging.exception` outside exception handler

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
LOG004 3 3 0 0 0

@InSyncWithFoo
Copy link
Contributor

Regarding the second example, you can check the qualified name against lint.logger-objects.

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

Successfully merging this pull request may close these issues.

2 participants