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

Implement flake8-logging #7248

Open
9 of 15 tasks
LaBatata101 opened this issue Sep 8, 2023 · 9 comments
Open
9 of 15 tasks

Implement flake8-logging #7248

LaBatata101 opened this issue Sep 8, 2023 · 9 comments
Labels
plugin Implementing a known but unsupported plugin

Comments

@LaBatata101
Copy link
Contributor

LaBatata101 commented Sep 8, 2023

Issue to track the implementation of flake8-logging rules.

  • LOG001 use logging.getLogger() to instantiate loggers
  • LOG002 use __name__ with getLogger()
  • LOG003 extra key '<key>' clashes with LogRecord attribute / Covered by G101
  • LOG004 avoid exception() outside of exception handlers
  • LOG005 use exception() within an exception handler / Covered by TRY400
  • LOG006 redundant exc_info argument for exception() / Covered by G202
  • LOG007 use error() instead of exception() with exc_info=False
  • LOG008 warn() is deprecated, use warning() instead / Covered by G010
  • LOG009 WARN is undocumented, use WARNING instead
  • LOG010 exception() does not take an exception
  • LOG011 avoid pre-formatting log messages / Covered by G004, G003, G002, G001
  • LOG012 formatting error: <n> <style> placeholders but <m> arguments
  • LOG013 formatting error: <missing/unreferenced> keys: <keys>
  • LOG014 avoid exc_info=True outside of exception handlers
  • LOG015 avoid logging calls on the root logger
@zanieb zanieb added the plugin Implementing a known but unsupported plugin label Sep 8, 2023
@qdegraaf
Copy link
Contributor

qdegraaf commented Sep 8, 2023

I'm implementing LOG007 but will wait for review and merge of #7249 and the boilerplate is in so it's easier to review

@LaBatata101
Copy link
Contributor Author

I'm implementing LOG001, just waiting for #7249 to get merged

@jonprindiville
Copy link

Hm, LOG001 feels a bit like a specific case of banned-api (TID251).

Not to say that LOG001 can't have an explicit implementation, just reminds me of this other thing.

charliermarsh pushed a commit that referenced this issue Sep 15, 2023
…OG009` (#7249)

## Summary

Adds `LOG009` from
[flake8-logging](https://github.com/adamchainz/flake8-logging). Also
adds the boilerplate for a new plugin

Checks for usages of undocumented `logging.WARN` constant and suggests
replacement with `logging.WARNING`.

## Test Plan

`cargo test` with fresh fixture

## Issue links

Refers: #7248
@qdegraaf
Copy link
Contributor

It looks like LOG012 is covered by PLE1206|LoggingTooFewArgs it only appears to cover %s formatting but so does LOG012 according to upstream README

@tjkuson
Copy link
Contributor

tjkuson commented Sep 15, 2023

LOG007 should probably be implemented only once #4136 is resolved; otherwise, it conflicts with TRY400.

charliermarsh pushed a commit that referenced this issue Sep 16, 2023
…7399)

## Summary

This PR implements a new rule for `flake8-logging` plugin that checks
for
`logging.getLogger` calls with either `__file__` or `__cached__` as the
first
argument and generates a suggested fix to use `__name__` instead.

Refer: #7248

## Test Plan

Add test cases and `cargo test`
charliermarsh pushed a commit that referenced this issue Sep 18, 2023
## Summary

This PR implements a new rule for `flake8-logging` plugin that checks
for uses of `logging.exception()` with `exc_info` set to `False` or a
falsy value. It suggests using `logging.error` in these cases instead.

I am unsure about the name. Open to suggestions there, went with the
most explicit name I could think of in the meantime.

Refer #7248

## Test Plan

Added a new fixture cases and ran `cargo test`
@dhruvmanila
Copy link
Member

Thanks! I've updated the issue description.

@javulticat
Copy link

Might I suggest that ruff drops/ignores flake8-logging-format (and the G error codes) entirely in favor of using flake8-logging (and exclusively the LOG error codes) as its source-of-truth for logging-related linting rules?

Currently, it is pretty confusing that ruff has logging-related linting rules spread out in two separate places (G and LOG). To an end user of ruff, there really shouldn't be two separate places to find logging-related linting rules - it really seems like it's just an implementation detail that they were originally derived from two separate projects.

To illustrate a concrete example of how this is a problem for our organization, I see here that you've marked that some of flake8-logging's LOG rules are already covered by some G rules from flake8-logging-format you already have, so you aren't planning on implementing those LOG rules (from what I gather, please correct me if I am wrong). As an organization that is currently using flake8 with flake8-logging, if we were to switch to ruff, how are our engineers supposed to know which of ruff's G rules map to the LOG rules they are familiar with (short of having our engineers digging through Github and finding this Issue, which isn't a realistic option)? I suppose you could put it in the official docs, but it would vastly reduce the friction for us to migrate to ruff if we could simply enable LOG rules and get the same behavior we currently get from flake8-logging. Because, even if the mapping from G to LOG rules was documented, is there any guarantee that the rules derived from flake8-logging-format have identical behavior as the corresponding rule(s) derived from flake8-logging?

That brings me to what is an even more important point, in my opinion. Compared to flake8-logging-format, flake8-logging seems to pretty objectively be a far better project to use as the source-of-truth from which you derive your logging-related linting rules. flake8-logging-format is "maintained" (and I use that word very lightly, because the project hasn't seen any activity in over a year) by some unknown corporate entity. On the other hand, flake8-logging was created and is actively maintained (most recent commit is less than a week old) by one of the most respected developers in the world of OSS Python software, adamchainz (whose Github profile should speak for itself, but highlights: core member of both Django and pytest teams, primary author/maintainer of many other highly regarded and heavily used OSS Python projects - including some I believe ruff is already using, such as flake8-comprehensions).

I encourage you to read Adam's blog post on why he created flake8-logging. Essentially, it sounds like he had his own concerns about using flake8-logging-format to enforce logging-related linting rules and decided to create a higher-quality, up-to-date flake8 plugin that contained a superset of those rules that would render flake8-logging-format obsolete: flake8-logging. So, if I understand that correctly, I believe you should be able to look at flake8-logging exclusively for (likely higher-quality) implementations of all of the rules that flake8-logging-format supports, plus all of the additional rules that flake8-logging has added (and will continue to add if/when helpful new rules are identified, such as the recent LOG013 and LOG014 rules, which is something flake8-logging-format hasn't done in years).

Thanks for considering! I've had a lot of my engineers ask about switching to ruff, and we've been watching the project with great interest, but this is definitely one of the larger blockers that are still preventing us from adopting it in my organization.

@jakkdl
Copy link

jakkdl commented Mar 24, 2024

New rule LOG015 added upstream: https://github.com/adamchainz/flake8-logging?tab=readme-ov-file#log015-avoid-logging-calls-on-the-root-logger

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin Implementing a known but unsupported plugin
Projects
None yet
Development

No branches or pull requests

9 participants