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

Add LOG015 to detect use of the root logger #96

Merged
merged 8 commits into from
Mar 20, 2024

Conversation

jakkdl
Copy link
Contributor

@jakkdl jakkdl commented Mar 19, 2024

The implementation of the check itself is super simple. But unsurprisingly it gives errors on tons of the examples on the other test checks. I first resolved it in TestLOG003 by actually adding the errors, but as you can see in the first commit that's not really tenable - so I instead added a way to ignore errors when running tests.

None of the TestLOG009 cases gives LOG015 since it only checks the calls in logger_methods. I don't think it's worth being that thorough though, but would be straightforward to extend the check.

It also remains a decision point on whether this check should be enabled by default or not, if you want me to make it ignored by default I can implement that as well.

@jakkdl
Copy link
Contributor Author

jakkdl commented Mar 19, 2024

Fixes #87

@adamchainz
Copy link
Owner

Please add a changelog note.

@Zac-HD
Copy link

Zac-HD commented Mar 19, 2024

As a complementary check, I'd suggest expanding LOG002 to warn on None.

@jakkdl
Copy link
Contributor Author

jakkdl commented Mar 20, 2024

As a complementary check, I'd suggest expanding LOG002 to warn on None.

I think that depends on whether logging.root or logging.getLogger(None) should be considered the canonical way of explicitly saying you want the root logger (and that explicitly wanting the root logger is a valid thing to want in a non-trivial amount of cases). As noted by adamchainz in #87 logging.root does not seem to be mentioned in the online docs at all, only place I can find a mention of it is when calling help(logging.Logger):

...

 |  ----------------------------------------------------------------------
 |  Data and other attributes defined here:
 |  
 |  manager = <logging.Manager object>
 |  
 |  root = <RootLogger root (WARNING)>
 |  
 |  ----------------------------------------------------------------------

...

So idk if we should recommend it? Or maybe this is just a case of the online docs being subpar and there being 0 risk that it gets deprecated or anything in the future.

If wanting the root logger is bad 99% of the time then outlawing ~all ways of accessing it and requiring # noqa to silence the warning seems fine, but otherwise we probably want a way to explicitly request it that doesn't raise any warnings.

Of course also an option to add a separate opt-in check for logging.getLogger(None) and/or logging.root

@jakkdl
Copy link
Contributor Author

jakkdl commented Mar 20, 2024

Please add a changelog note.

done! ✔️

Copy link
Owner

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gonna fix up and merge this now, some review with changes that I will apply.

@adamchainz adamchainz force-pushed the calls_on_root_logger branch from fb107c1 to a2e6515 Compare March 20, 2024 20:38
@adamchainz adamchainz changed the title add LOG015: avoid logging calls on root logger. Ignore it in most tests. Add LOG015 to detect use of the root logger Mar 20, 2024
Comment on lines +202 to +206
) or (
isinstance(node.func, ast.Name)
and node.func.id in logger_methods
and self._from_imports.get(node.func.id) == "logging"
):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expanded the check to include constructs like from logging import info because it was easy, though I don't think there will be much use of that pattern

]


run_ignore_log015 = partial(run, ignore=("LOG015",))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I preferred an alias function rather than adding a base class. No need for inheritance here.

Comment on lines +1719 to +1728
def test_root_call_alias(self):
results = run(
"""\
import logging as loglog
loglog.info(...)
"""
)
assert results == [
(2, 0, "LOG015 avoid logging calls on the root logger"),
]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split this test in two, the second part being test_logger_call. Tests should cover only one thing, so they can fail atomically.

@adamchainz adamchainz merged commit 55836ac into adamchainz:main Mar 20, 2024
7 checks passed
@adamchainz
Copy link
Owner

Released in 1.6.0. Great work, thank you!

@jakkdl
Copy link
Contributor Author

jakkdl commented Mar 22, 2024

Thank you for thorough review & cleanup!

@jakkdl jakkdl deleted the calls_on_root_logger branch March 22, 2024 15:06
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.

3 participants