Fix logging-fstring-interpolation false positive
#7846
Conversation
Pierre-Sassoulas
added
False Positive 🦟
pylint/checkers/logging.py
Outdated
| elif isinstance(node.args[format_pos], nodes.JoinedStr): | ||
| elif isinstance(format_arg, nodes.JoinedStr): | ||
| if any( | ||
| "%s" in val.value |
There was a problem hiding this comment.
There's a lot of potential other formatting here %r %d, %0.3f, %10s... Maybe this isn't the right approach.
There was a problem hiding this comment.
Yeah that's what I assumed I'd hear back. I figured '%s' is the most common, maybe d, r, and we could at least get rid of that FP. Any better way to detect %x formatting?
There was a problem hiding this comment.
I'm not sure but I think it's not easy to solve. @DanielNoord you worked a lot on f-string could you confirm ?
There was a problem hiding this comment.
Yeah this seems to open a can of worms, although the same could be said for the original issue.
I don't know, we might just try this and see if other requests pop up.
There was a problem hiding this comment.
Unless we think the perf hit is egregious (which I don't since this is in a very specific code section), having this code or maybe I could refactor to a well named method _check_str_formatting or something, and can add a few more formatting types like %d or %r which should cover probably 95% of what we'd ever see in this pattern.
There was a problem hiding this comment.
+1 for checking only the most common one. The primer already show that this is useful. Let's do %s, %d, %f and %r. Probably something like is '%' in val.value and any(x in val.value for x in [%s, %d, %f, %r]). Not sure if it's the best performance wise but probably better than any(x in val.value for x in [%s, %d, %f, %r])
Pull Request Test Coverage Report for Build 3553960859
💛 - Coveralls |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Mark Byrne <31762852+mbyrnepr2@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
🤖 Effect of this PR on checked open source code: 🤖 Effect on django:
This comment was generated for commit d4403c3 |
Pierre-Sassoulas
changed the title
logging-fstring-interpolation FPlogging-fstring-interpolation false positive
Co-authored-by: Mark Byrne <31762852+mbyrnepr2@users.noreply.github.com> Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com> (cherry picked from commit 978d1ab)
Co-authored-by: Mark Byrne <31762852+mbyrnepr2@users.noreply.github.com> Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com> (cherry picked from commit 978d1ab) Co-authored-by: Dani Alcala <112832187+clavedeluna@users.noreply.github.com>
Type of Changes
Description
logging-fstring-interpolationshould not be raised if logging with an f-str that also has %s formatting.Closes #4984