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

Incorrect f-string application #6889

Closed
hotpxl opened this issue Aug 26, 2023 · 9 comments
Closed

Incorrect f-string application #6889

hotpxl opened this issue Aug 26, 2023 · 9 comments
Labels
bug Something isn't working

Comments

@hotpxl
Copy link

hotpxl commented Aug 26, 2023

Python file

a = 1
b = ("{{hello}}" "{}").format(a)

Command

target/debug/ruff check --select UP032 --fix --diff a.py

Vanilla Ruff, no settings, built from master

commit f91bacbb94e1ba0dc56b15afe388fe1f6c92e3e8 (HEAD -> main, origin/main, origin/HEAD)
Author: Charlie Marsh <charlie.r.marsh@gmail.com>
Date:   Fri Aug 25 19:47:15 2023 -0400

The ruff output is

--- a.py
+++ a.py
@@ -1,2 +1,2 @@
 a = 1
-b = ("{{hello}}" "{}").format(a)
+b = ("{{hello}}" f"{a}")

which is incorrect. The old code, when calling .format, collapses the double braces in {{hello}}. But the Ruff version doesn't

@zanieb
Copy link
Member

zanieb commented Aug 26, 2023

Hi @hotpxl on what version did this used to work? Could you help us determine when this regression was introduced?

@hotpxl
Copy link
Author

hotpxl commented Aug 26, 2023

I don't know if any version used to work, but I can try and git bisect

realized my original description is misleading, corrected the wording

@dhruvmanila
Copy link
Member

It seems like the case of implicit string concatenation and as there are no placeholder in the first string, it's not being considered. If you change the code to b = ("{{hello}}{}" "{}").format(a, a), the fix is correct. I guess the fix could be to add the f prefix to all strings within (...).format() call?

@charliermarsh
Copy link
Member

Yeah, we probably need to add the f prefix to all segments in the implicit concat, is my guess.

@charliermarsh charliermarsh self-assigned this Aug 28, 2023
@charliermarsh charliermarsh added the bug Something isn't working label Aug 28, 2023
@charliermarsh charliermarsh removed their assignment Aug 28, 2023
@raylu
Copy link

raylu commented Sep 28, 2023

another example:

print('{{ ' '{} {}'.format(1, 2))
$ ruff check --select UP032 test.py --diff
--- test.py
+++ test.py
@@ -1 +1 @@
-print('{{ ' '{} {}'.format(1, 2))
+print('{{ ' f'{1} {2}')

the correct version is print('{ ' f'{1} {2}')

$ ruff --version
ruff 0.0.291

@MichaReiser
Copy link
Member

Yeah, we probably need to add the f prefix to all segments in the implicit concat, is my guess.

Won't that potentially trigger another violation because some parts now use f-strings unnecessarily?

@raylu
Copy link

raylu commented Sep 29, 2023

surprisingly, no

print(f'{{ ')
$ ruff check . --select F541
test.py:1:7: F541 [*] f-string without any placeholders

but

print(f'{{ ' f'{1} {2}')
$ ruff check . --select F541
[no output]

but even if it did, F541 is autofixable too. just gotta autofix in the right order

@charliermarsh
Copy link
Member

No, we don't flag pieces of an implicit concatenation, if at least one segment contains a placeholder. (This is intentional.)

@AlexWaygood
Copy link
Member

Looks like this was fixed back in November in #8697 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants