Remove NBSP at the beggining of comments

[Pierre-Sassoulas]

This is a fast attempt to do #2091. I suppose we could remove the while and put a simple if to not remove multiple consecutive NBSP.

Closes #2091

[ichard26]

Looking into this, the reason why Python 3.6 and 3.7 are failing is probably because typed_ast and ast doesn't recognize "#\N{NO-BREAK SPACE}type: ignore" as an actual type ignore comment. The reason why only 3.6 and 3.7 is failing is because typed_ast has a special field for type ignore comments in the AST. The built-in parser (ast) which is used on Python 3.8 and higher has its type ignore comment specific tracking features disabled which means the "apparently invalid type ignore comment turning into valid one" effect goes unnoticed.

[JelleZijlstra]

Thanks for the diagnosis @ichard26! I think for safety we should just leave any nbsps in type comments alone.

[Pierre-Sassoulas]

@JelleZijlstra I believe it's now ready for review. Thanks @ichard26 for dong all the leg work :) !

[ichard26]

FYI, you can also use Unicode name escapes, so instead of a defining an aptly named variable, you could just replace it with "\N{NO-BREAK SPACE}"

Now does it matter? Probably not since it's arguable which one is more readable, but I'd like to point it out to you as a language feature you may have not known :)

[Pierre-Sassoulas]

Thanks I did not know ! 😄

[JelleZijlstra]

Thanks! The code looks good now, but I think the tests will be cleaner in a separate file.

[JelleZijlstra]

iff is correct here, it means "if and only if" (https://en.wikipedia.org/wiki/If_and_only_if)

[Pierre-Sassoulas]

My bad 😅

[JelleZijlstra]

It's going to be hard for readers of this file to recognize what's going on here, because nbsps look the same as spaces pretty much all the time, at least in environments I'm familiar with.

What about undoing the changes to this file and instead adding a new test like nbsp.py just for testing comments with nbsps in them? You'll have to add a new test function in test_black.py, but that's pretty easy.

[Pierre-Sassoulas]

👍

[Pierre-Sassoulas]

I copy pasted comment7.py with a new name and added NBSP as the first character of each comments. The reflexion is that there was a lot of different test cases in comment7 but let me know if simple test cases (normal comment, commen with a lot of leading spaces, type ignore comment, type: Optional[Squre] comment ?) is preferable.

[JelleZijlstra]

Suggested change

	- `Black` now remove leading non-breaking spaces in comments before adding a space
	- `Black` now cleans up leading non-breaking spaces in comments

I think this is better; we don't need to be too specific about the exact output.

[JelleZijlstra]

Could you remove some extraneous code here? I think most of the imports on this line can go for example, and most of the cases in class C below aren't really relevant either. I'd want this test file to have only comments with nbsps and elements that are directly related (e.g., to test behavior around moving comments).

[JelleZijlstra]

Thanks!

[Pierre-Sassoulas]

I made the test a little too nasty and found a bug:

def function(a:int=42):
    """ This docstring start with a NBSP
        There is one column 5 here
        This is 7 NBSP
    """

Error log:

--- src
+++ dst
@@ -162,11 +162,11 @@
       body=
         Expr(
           value=
             Constant(
               value=
-                'This docstring start with a NBSP\n\xa0   There is one column 5 here\n\xa0\xa0\xa0\xa0This is 7 NBSP',  # str
+                'This docstring start with a NBSP\nThere is one column 5 here\nThis is 7 NBSP',  # str
             )  # /Constant
         )  # /Expr
         Pass(
         )  # /Pass
       decorator_list=

Should I open a new issue or address it here ?

[JelleZijlstra]

Oops, too late. Could you just file a new PR?