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

Regression: Unstable formatting on long strings #1042

Closed
JelleZijlstra opened this issue Oct 3, 2019 · 2 comments · Fixed by #1079
Closed

Regression: Unstable formatting on long strings #1042

JelleZijlstra opened this issue Oct 3, 2019 · 2 comments · Fixed by #1079

Comments

@JelleZijlstra
Copy link
Collaborator

I tested Black master on our codebase in preparation for a release, but found that Black failed to format 17 files (the previous release didn't crash on any of our files). It's not clear that all of these have the same root cause, but here's one I minimized somewhat:

$ black infra-common/qroxy/qroxy.py 
error: cannot format infra-common/qroxy/qroxy.py: INTERNAL ERROR: Black produced different code on the second pass of the formatter.  Please report a bug on https://github.com/psf/black/issues.  This diff might be helpful: /tmp/blk_i040l_hj.log
Oh no! 💥 💔 💥
1 file failed to reformat.
$ cat /tmp/blk_i040l_hj.log 
--- source
+++ first pass
@@ -1,10 +1,10 @@
 async def show_status():
     while True:
         try:
             if report_host:
                 data = (
-                    f"{boxname},{alive_server_connections},{server_queue.size()},{requester_queue.size()}"
-                ).encode()
+                    f"{boxname},{alive_server_connections},{server_queue.size()},{requester_queue.size()}".encode()
+                )
         except Exception as e:
             log.error(f"show_status thread encounters exception: {e}")
 
--- first pass
+++ second pass
@@ -1,10 +1,8 @@
 async def show_status():
     while True:
         try:
             if report_host:
-                data = (
-                    f"{boxname},{alive_server_connections},{server_queue.size()},{requester_queue.size()}".encode()
-                )
+                data = f"{boxname},{alive_server_connections},{server_queue.size()},{requester_queue.size()}".encode()
         except Exception as e:
             log.error(f"show_status thread encounters exception: {e}")
 
$ cat infra-common/qroxy/qroxy.py 
async def show_status():
    while True:
        try:
            if report_host:
                data = (
                    f"{boxname},{alive_server_connections},{server_queue.size()},{requester_queue.size()}"
                ).encode()
        except Exception as e:
            log.error(f"show_status thread encounters exception: {e}")

@hugovk
Copy link
Contributor

hugovk commented Oct 13, 2019

Like #1044, git bisect points to the same 957ba24 in PR #850 to fix #548:

957ba24bb6cdd32e4dd14ff2808dcf4d58851844 is the first bad commit
commit 957ba24bb6cdd32e4dd14ff2808dcf4d58851844
Date:   Wed May 15 21:11:04 2019 -0700

    remove obviously unnecessary parentheses (#850)

    Fixes #548

:100644 100644 17aea7af264adafc4f5ee18cb9389cdfcdce8e17 694c1e961aa41bef489ea08145f1ee9faed7c69b M	black.py
:040000 040000 8da77aae59466f50234b0c16138cfcc5e88ebc17 3d7f6604495d23cbe68f30c434cf0d7a8ad1779d M	tests
bisect run success

Again, the second pass is stable and leads to no further formatting.

@JelleZijlstra
Copy link
Collaborator Author

Sounds like we should back out that change.

JelleZijlstra added a commit that referenced this issue Oct 20, 2019
Fixes #1042 (and probably #1044 which looks like the same thing).

The issue with the "obviously unnecessary" parentheses that #850 removed is that sometimes they're necessary to help Black fit something in one line. I didn't see an obvious solution that still removes the parens #850 was intended to remove, so let's back out this change for now in the interest of unblocking a release.

This PR also adds a test adapted from the failing example in #1042, so that if we try to reapply the #850 change we don't break the same case again.
amyreese pushed a commit to amyreese/black that referenced this issue May 7, 2020
Fixes psf#1042 (and probably psf#1044 which looks like the same thing).

The issue with the "obviously unnecessary" parentheses that psf#850 removed is that sometimes they're necessary to help Black fit something in one line. I didn't see an obvious solution that still removes the parens psf#850 was intended to remove, so let's back out this change for now in the interest of unblocking a release.

This PR also adds a test adapted from the failing example in psf#1042, so that if we try to reapply the psf#850 change we don't break the same case again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants