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

PEP 701 support breaks stability policy #4324

Closed
hauntsaninja opened this issue Apr 22, 2024 · 10 comments · Fixed by #4325
Closed

PEP 701 support breaks stability policy #4324

hauntsaninja opened this issue Apr 22, 2024 · 10 comments · Fixed by #4325
Labels
T: bug Something isn't working

Comments

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Apr 22, 2024

#3822 is amazing work. I've been sort of behind on all open source stuff, so I only just got to running it on my codebase at work. I found some reformatting, which violates the stability policy.

def foo():
    class aaa(bbbb):
        value: str = f"""
        fdsa
        asdf
        ```section
        {value}
        ```
        """

There were eleven instances of things that got changed

@hauntsaninja hauntsaninja added the T: bug Something isn't working label Apr 22, 2024
@hauntsaninja
Copy link
Collaborator Author

Oh actually this one is a maybe a little different (with line length 100):

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,
                )

Reformatting collapses the f-strings onto one line, which seems undesirable

@hauntsaninja
Copy link
Collaborator Author

cc @JelleZijlstra for awareness. If we need to break stability policy to get this out, it's worth it, but maybe it's not too hard to fix.

@JelleZijlstra
Copy link
Collaborator

cc @tusharsadhwani.

What are you seeing exactly on your first example? I tried running current Black main (on Python 3.12.1) on a file with your code sample and didn't get any changes.

@JelleZijlstra
Copy link
Collaborator

I can reproduce the second one though:

 % python -m black --diff src/black/shantanu2.py -l 100
--- src/black/shantanu2.py	2024-04-22 23:36:08.610531+00:00
+++ src/black/shantanu2.py	2024-04-22 23:36:27.151299+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 {self.writer._transport.get_extra_info('peername')}",  # type: ignore[attr-defined]
                     level=0,
                 )
would reformat src/black/shantanu2.py

All done! ✨ 🍰 ✨
1 file would be reformatted.

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Apr 22, 2024

For the first one, you need to not have unstable = true (if you run inside black's directory you'll pick up unstable = true automatically). I just delete black's pyproject.toml

--- b.py	2024-04-22 23:47:32.201559+00:00
+++ b.py	2024-04-22 23:49:26.995636+00:00
@@ -1,9 +1,11 @@
 def foo():
     class aaa(bbbb):
-        value: str = f"""
+        value: str = (
+            f"""
         fdsa
         asdf
         ```section
         {value}
         ```
         """
+        )
would reformat b.py

@tusharsadhwani
Copy link
Contributor

tusharsadhwani commented Apr 23, 2024

I'm thinking it has to do with code that was special cased for STRING tokens, but now there's FSTRING_START tokens there.

Code like this:

black/src/black/brackets.py

Lines 263 to 268 in 551ede2

if (
leaf.type == token.STRING
and previous is not None
and previous.type == token.STRING
):
return STRING_PRIORITY

Interesting thing about second case is that the issue goes away if you remove the type ignore comment.

@JelleZijlstra
Copy link
Collaborator

I'm planning to hold off on making a release until this is either fixed or we've investigated enough to convince ourselves that it's very difficult to fix.

I'll try to find some time to look into this myself too; @tusharsadhwani is probably right that it's about STRING special case that now also needs to look at f-string tokens. If so, the fix shouldn't be difficult.

@hauntsaninja
Copy link
Collaborator Author

(in case useful for prioritising, I think the second one is much rarer, all the other hits in my work codebase look like the first case)

@tusharsadhwani
Copy link
Contributor

tusharsadhwani commented Apr 23, 2024

@hauntsaninja I'm unable to reproduce the first case, can you provide the command you're running?

EDIT: nevermind. I got it to reproduce.

@hauntsaninja
Copy link
Collaborator Author

/tmp λ cat asd.py                          
def foo():
    class aaa(bbbb):
        value: str = f"""
        fdsa
        asdf
        ```section
        {value}
        ```
        """
/tmp λ /Users/shantanu/.virtualenvs/black-drsy/bin/python -m black asd.py --check --diff
--- asd.py	2024-04-23 19:21:00.283019+00:00
+++ asd.py	2024-04-23 19:21:33.367713+00:00
@@ -1,9 +1,11 @@
 def foo():
     class aaa(bbbb):
-        value: str = f"""
+        value: str = (
+            f"""
         fdsa
         asdf
         ```section
         {value}
         ```
         """
+        )
would reformat asd.py

Oh no! 💥 💔 💥
1 file would be reformatted.

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

Successfully merging a pull request may close this issue.

3 participants