Skip to content

Prevent wrapping of multiline fstrings in parens#4325

Merged
JelleZijlstra merged 7 commits intopsf:mainfrom
tusharsadhwani:fstring-bracketing
Apr 23, 2024
Merged

Prevent wrapping of multiline fstrings in parens#4325
JelleZijlstra merged 7 commits intopsf:mainfrom
tusharsadhwani:fstring-bracketing

Conversation

@tusharsadhwani
Copy link
Collaborator

@tusharsadhwani tusharsadhwani commented Apr 23, 2024 •
edited
Loading

Resolves #4324

Description

The function is_multiline_string was only being run for Leaf nodes, but in that context, multiline f-strings should also have been respected. The code has been adjusted to respect f-strings as well.

The STRING leaves created by fstring_to_string were also missing lineno fields which are used in parts of the codebase.

@ichard26 ichard26 added the ci: skip news Pull requests that don't need a changelog entry. label Apr 23, 2024
@tusharsadhwani tusharsadhwani changed the title prevent wrapping of multiline fstrings in parens Prevent wrapping of multiline fstrings in parens Apr 23, 2024
@tusharsadhwani
Copy link
Collaborator Author

tusharsadhwani commented Apr 23, 2024

The one other edge case in the issue is also likely around this function:

black/src/black/lines.py

Line 276 in 551ede2

def contains_uncollapsable_type_comments(self) -> bool:

I'm seeing if that's a small fix that can go in the same PR.

@tusharsadhwani
Copy link
Collaborator Author

tusharsadhwani commented Apr 23, 2024

^ So that other edge case is not related to #3822, as I'm able to reproduce it even on black v24.4.0:

$ black --version                 
black, 24.4.0 (compiled: yes)
Python (CPython) 3.12.2
$ cat asd.py
log(
    f"Received operation {server_operation.name} from "
    f"{self.writer._transport.get_extra_info('peername')}",  # type: ignore[attr-defined]
)
$ black asd.py
reformatted asd.py

All done! ✨ 🍰 ✨
1 file reformatted.
$ cat asd.py 
log(
    f"Received operation {server_operation.name} from {self.writer._transport.get_extra_info('peername')}",  # type: ignore[attr-defined]
)

cc @hauntsaninja, does that seem right?

@github-actions
Copy link

github-actions bot commented Apr 23, 2024 •
edited
Loading

diff-shades reports zero changes comparing this PR (b5df1ae) to main (551ede2).

What is this? | Workflow run | diff-shades documentation

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Apr 23, 2024

No, that doesn't seem right.

Make sure you're not getting fooled by unstable = true (which you'll pick up by default if the file is in black project dir).

(black-drsy) ~/dev/black 551ede2 λ cat /tmp/b.py 
def a():
    def b():
        def c():
            def d():
                log(
                    f"Received operation {server_operation.name} from "
                    f"{self.writer._transport.get_extra_info('peername')}",  # type: ignore[attr-defined]
                    level=0,
                )
(black-drsy) ~/dev/black 551ede2 λ python -m black /tmp/b.py -l 100 --check --diff
--- /tmp/b.py	2024-04-23 20:23:56.523454+00:00
+++ /tmp/b.py	2024-04-23 20:24:23.464522+00:00
@@ -1,9 +1,8 @@
 def a():
     def b():
         def c():
             def d():
                 log(
-                    f"Received operation {server_operation.name} from "
-                    f"{self.writer._transport.get_extra_info('peername')}",  # type: ignore[attr-defined]
+                    f"Received operation {server_operation.name} from " f"{self.writer._transport.get_extra_info('peername')}",  # type: ignore[attr-defined]
                     level=0,
                 )
would reformat /tmp/b.py

Oh no! 💥 💔 💥
1 file would be reformatted.
(black-drsy) ~/dev/black 551ede2 λ gco HEAD~
Previous HEAD position was 551ede2 Add PEP 701 support (#3822)
HEAD is now at 944b99a Bump sphinx from 7.2.6 to 7.3.7 in /docs (#4322)
(black-drsy) ~/dev/black 944b99a λ python -m black /tmp/b.py -l 100 --check --diff
All done! ✨ 🍰 ✨
1 file would be left unchanged.
(black-drsy) ~/dev/black 944b99a λ

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Apr 23, 2024

And sorry for not fully minimising it, e.g. this repros without -l 100 for me:

log(
    f"Received operation {server_operation.name} from "
    f"{self.writer._transport.get_extra_info('peername')}",  # type: ignore[attr-defined]
    level=0,
)

@tusharsadhwani
Copy link
Collaborator Author

tusharsadhwani commented Apr 23, 2024

Thanks for clarification, both cases should be fixed now.

hauntsaninja
hauntsaninja approved these changes Apr 23, 2024
View reviewed changes
Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

This looks great. Thank you for the fixes (and thank you again for the original feature)!

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Apr 23, 2024

(also confirmed this commit is clean on my current work codebase)

JelleZijlstra
JelleZijlstra approved these changes Apr 23, 2024
View reviewed changes
Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks!

@JelleZijlstra JelleZijlstra merged commit f4b644b into psf:main Apr 23, 2024
@data-sync-user data-sync-user mentioned this pull request Jun 3, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@hauntsaninja hauntsaninja hauntsaninja approved these changes

Assignees

No one assigned

Labels

ci: skip news Pull requests that don't need a changelog entry.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

PEP 701 support breaks stability policy

4 participants

@tusharsadhwani @hauntsaninja @JelleZijlstra @ichard26