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

Prevent wrapping of multiline fstrings in parens #4325

Merged
merged 7 commits into from
Apr 23, 2024

Conversation

tusharsadhwani
Copy link
Contributor

@tusharsadhwani tusharsadhwani commented Apr 23, 2024

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 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
Contributor Author

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

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
Contributor Author

^ 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?

Copy link

github-actions bot commented Apr 23, 2024

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


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

@hauntsaninja
Copy link
Collaborator

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

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
Contributor Author

Thanks for clarification, both cases should be fixed now.

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

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

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
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news Pull requests that don't need a changelog entry.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PEP 701 support breaks stability policy
4 participants