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.

Sign up for GitHub

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

Jump to bottom

Fix logging-fstring-interpolation false positive #7846

Merged
merged 5 commits into from Nov 26, 2022
Merged

Fix logging-fstring-interpolation false positive #7846

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7d9b6b9
Fix ``logging-fstring-interpolation``
clavedeluna Nov 25, 2022
676a4bb
check if s formatting in f str
clavedeluna Nov 25, 2022
d483c83
Apply suggestions from code review
clavedeluna Nov 25, 2022
b958d0a
Update pylint/checkers/logging.py
Pierre-Sassoulas Nov 26, 2022
d4403c3
Update pylint/checkers/logging.py
Pierre-Sassoulas Nov 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions doc/whatsnew/fragments/4984.false_positive
View file
Open in desktop
@@ -0,0 +1,3 @@
Fix ``logging-fstring-interpolation`` false positive raised when logging and f-string with ``%s`` formatting.

Closes #4984
20 changes: 14 additions & 6 deletions pylint/checkers/logging.py
View file
Open in desktop
Expand Up @@ -240,8 +240,9 @@ def _check_log_method(self, node: nodes.Call, name: str) -> None:
else:
return

if isinstance(node.args[format_pos], nodes.BinOp):
binop = node.args[format_pos]
format_arg = node.args[format_pos]
if isinstance(format_arg, nodes.BinOp):
binop = format_arg
emit = binop.op == "%"
if binop.op == "+":
total_number_of_strings = sum(
Expand All @@ -256,11 +257,18 @@ def _check_log_method(self, node: nodes.Call, name: str) -> None:
node=node,
args=(self._helper_string(node),),
)
elif isinstance(node.args[format_pos], nodes.Call):
self._check_call_func(node.args[format_pos])
elif isinstance(node.args[format_pos], nodes.Const):
elif isinstance(format_arg, nodes.Call):
self._check_call_func(format_arg)
elif isinstance(format_arg, nodes.Const):
self._check_format_string(node, format_pos)
elif isinstance(node.args[format_pos], nodes.JoinedStr):
elif isinstance(format_arg, nodes.JoinedStr):
if any(
"%s" in val.value
Copy link
Member

Choose a reason for hiding this comment

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

There's a lot of potential other formatting here %r %d, %0.3f, %10s... Maybe this isn't the right approach.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure but I think it's not easy to solve. @DanielNoord you worked a lot on f-string could you confirm ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

+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])

for val in format_arg.values
if isinstance(val, nodes.Const)
):
# Detect % formatting inside an f-string, such as `f'Hello %s'`
return
self.add_message(
"logging-fstring-interpolation",
node=node,
Expand Down
5 changes: 5 additions & 0 deletions tests/functional/l/logging/logging_fstring_interpolation_py37.py
View file
Open in desktop
Expand Up @@ -3,3 +3,8 @@

VAR = "string"
logging.error(f"{VAR}") # [logging-fstring-interpolation]

WORLD = "world"
logging.error(f'Hello {WORLD}') # [logging-fstring-interpolation]
clavedeluna marked this conversation as resolved.
Show resolved Hide resolved

logging.error(f'Hello %s', 'World!') # [f-string-without-interpolation]
2 changes: 2 additions & 0 deletions tests/functional/l/logging/logging_fstring_interpolation_py37.txt
View file
Open in desktop
@@ -1 +1,3 @@
logging-fstring-interpolation:5:0:5:23::Use lazy % formatting in logging functions:UNDEFINED
logging-fstring-interpolation:8:0:8:31::Use lazy % formatting in logging functions:UNDEFINED
f-string-without-interpolation:10:14:10:25::Using an f-string that does not have any interpolated variables:UNDEFINED