[ruff] Add logging-eager-conversion (RUF065)
#19942
Conversation
GDYendell
force-pushed
the
logging-str-preformat
branch
2 times, most recently
from
bdf1eab to
1de7082
Compare
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RUF065 | 131 | 131 | 0 | 0 | 0 |
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thank you! This looks good to me overall, I mostly had small nit comments.
Otherwise, I'm not totally sure if we can make this a G rule. I see an upstream issue was opened last August, at the same time as the Ruff issue (globality-corp/flake8-logging-format#79). There hasn't been a response there specifically, but there has been activity in the repo in the last 3 months. The rule fits really nicely here with the other G rule implementations, as well as thematically, but we also don't want to run into rule conflicts down the line. @MichaReiser do we need to make this a RUF rule to be safe?
I'm also not totally sold on the name. What do you think about LoggingEagerConversion? Just an idea, I'm not sure I love that either.
Finally, would you mind looking through the ecosystem results? The simple implementation here is very appealing, but we'll want to make sure there aren't any false positives that might warrant parsing and cross-referencing the formatting arguments, as Charlie mentioned on the issue.
crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_logging_format/violations.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_logging_format/violations.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_logging_format/violations.rs
Outdated
Show resolved
Hide resolved
|
Thanks for the review! I added a couple of fixup commits to address your comments.
I wasn't sure what to call it and chose the name in the hope that someone would come up with something better 😅 I prefer
Will do - this is a really cool CI helper! I have had a quick look and there are some casts of exceptions, which possibly do more the logging formatter does. |
|
I have had a look through the examples in the affected ecosystem packages and I don't see any issues. As a summary:
I didn't see any hits that aren't explicitly calling str or repr, so I think those are all correct matches. |
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks! I had one more small suggestion, and then I did find a potential issue in the ecosystem check. A case like this is currently flagged:
import logging
try:
1/0
except Exception as e:
logging.error("error: %s", e)That's no problem, the output is the same after dropping the str call:
>>> try:
... 1/0
... except Exception as e:
... logging.error("error: %s", str(e))
...
ERROR:root:error: division by zero
>>> try:
... 1/0
... except Exception as e:
... logging.error("error: %s", e)
...
ERROR:root:error: division by zero
>>>However, mixing %s and repr, does change the result:
>>> try:
... 1/0
... except Exception as e:
... logging.error("error: %s", repr(e))
...
ERROR:root:error: ZeroDivisionError('division by zero')
>>> try:
... 1/0
... except Exception as e:
... logging.error("error: %s", e)
...
ERROR:root:error: division by zeroI saw this in this langchain example from the ecosystem check. That's not a super big problem given the current diagnostic message, which doesn't directly tell you to drop the repr call, but we'd at least need to add an example to the docs where %r is used.
However, the reverse combination of %r and str might actually be out of scope for the rule because it changes the output in a way that I don't think we can easily capture (abbreviated example to make comparison easier):
logging.error("error: %r", e) # ERROR:root:error: ZeroDivisionError('division by zero')
logging.error("error: %r", str(e)) # ERROR:root:error: 'division by zero'
logging.error("error: %s", e) # ERROR:root:error: division by zeroNote the quotes in %r/str compared to just %s.
I think that means we need to go ahead and do the format string parsing and comparison with the positional arguments that Charlie mentioned in the issue. We would need to do that for a future autofix anyway, although it's fine to leave a fix itself for a follow-up PR. What do you think?
This might be less of a big deal than I'm imagining, but flagging cases where users need to change "%r", str(...) to "'%r'", ... feels like it could be confusing and less desirable compared to cases where you just drop a str or repr call.
Otherwise I think we just need to decide on the name and rule code!
crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs
Outdated
Show resolved
Hide resolved
Is your first snippet here supposed to have
Ah that is interesting, I hadn't appreciated that. This is a tricky one then because the user might prefer that output. They would want to use The try:
1/0
except Exception as e:
logging.error("error: %r", str(e))
ERROR:root:error: 'division by zero'try:
1/0
except Exception as e:
logging.error("error: '%r'", e)
ERROR:root:error: 'ZeroDivisionError('division by zero')' |
Ah yes, you're right, sorry!
I think I'd prefer to go ahead and give a tailored message for each case we know about, if it's not too much more work. I ran into a somewhat analogous case for another rule recently where we got reports about the confusing, generic help message. This looks like an example of similar parsing in another rule that doesn't look too bad: ruff/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs Lines 388 to 407 in 58efd19
Oops, good catch again. Yeah, we might want to avoid a diagnostic in this case. It seems like kind of a roundabout way to wrap something in quotes, but I still think it's intentional. We could always expand the rule later, but I'd lean more towards definite true positives like I also remembered that there's a vaguely similar rule for f-strings: explicit-f-string-type-conversion (RUF010). This replaces calls like |
|
OK I think I see how to do that. Just to confirm, we want to iterate over the placeholders in the format string with the corresponding values and only flag |
Yep, that's exactly what I had in mind! |
|
It now only flags %s with str() and %r with repr(). What do you think? Is it possible to provide two ranges to the diagnostic so that it underlines both %s and str()? |
That looks great!
Yes, as of ~last week! The easiest way is with ruff/crates/ruff_linter/src/checkers/ast/mod.rs Lines 3309 to 3311 in 2245355 Here's the one use of it we've added so far: ruff/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs Lines 195 to 198 in 2245355 And an example of the type of diagnostic it produces (the There are some other options if that doesn't look good, but that's the easiest place to start. |
|
Hmm. I am not sure this is possible because Thoughts? |
|
Yeah, I don't think it makes sense to underline the whole string. I guess I was assuming that Would you mind resolving the conflicts? Hopefully they're not too bad. I'll try to give this another review soon :) |
GDYendell
force-pushed
the
logging-str-preformat
branch
from
e5547d4 to
ab44c73
Compare
|
Squashed and rebased to resolve conflicts. |
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thank you! This looks great to me. I just had a few very small suggestions, and then I think we should move this to a Ruff rule. But this is otherwise good to go.
crates/ruff_linter/src/rules/flake8_logging_format/violations.rs
Outdated
Show resolved
Hide resolved
...ake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G005.py.snap
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_logging_format/violations.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs
Outdated
Show resolved
Hide resolved
GDYendell
force-pushed
the
logging-str-preformat
branch
from
f6665ec to
e8b6439
Compare
GDYendell
changed the title
GDYendell
force-pushed
the
logging-str-preformat
branch
from
e8b6439 to
229369c
Compare
GDYendell
changed the title
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Excellent, thank you! I managed to find a few more tiny nits, but this looks great.
crates/ruff_linter/src/rules/ruff/rules/logging_eager_conversion.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/logging_eager_conversion.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/logging_eager_conversion.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/logging_eager_conversion.rs
Outdated
Show resolved
Hide resolved
GDYendell
force-pushed
the
logging-str-preformat
branch
from
20d436a to
427ad1c
Compare
ntBre
changed the title
ruff] Add logging-eager-conversion (RUF065)
2 participants
Summary
Fixes #12734
I have started with simply checking if any arguments that are providing extra values to the log message are calls to
strorrepr, as suggested in the linked issue. There was a concern that this could cause false positives and the check should be more explicit. I am happy to look into that if I have some further examples to work with.If this is the accepted solution then there are more cases to add to the test and it should possibly also do test for the same behavior via the
extrakeyword.Test Plan
I have added a new test case and python file to flake8_logging_format with examples of this anti-pattern.