Remove NBSP at the beggining of comments #2092

Pierre-Sassoulas · 2021-04-07T18:57:20Z

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 · 2021-04-10T17:51:32Z

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 · 2021-04-10T18:53:21Z

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

See rational:https://github.com/psf/black/pull/2092\#issuecomment-817178166

The switch to unvalid 'type: ignore' to valid 'type: ignore' is a lot more complicated to do. See:https://github.com/psf/black/pull/2092\#issuecomment-817178166

Pierre-Sassoulas · 2021-04-11T09:43:54Z

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

src/black/__init__.py

tests/data/comments7.py

ichard26 · 2021-04-11T14:37:06Z

src/black/__init__.py

+    NON_BREAKING_SPACE = " "
+    if content and content[0] == NON_BREAKING_SPACE and "type: ignore" not in content:


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

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 · 2021-04-11T19:56:18Z

src/black/__init__.py

    """
    Returns:
-        True iff one of the comments in @comment_list is a pragma used by one
+        True if one of the comments in @comment_list is a pragma used by one


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

My bad 😅

JelleZijlstra · 2021-04-11T19:58:37Z

tests/data/comments7.py

    Path,
    #  String,
-    #  resolve_to_config_type,
+    #  resolve_to_config_type,


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.

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 · 2021-04-11T20:00:02Z

CHANGES.md


 #### _Black_

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


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.

The switch to unvalid 'type: ignore' to valid 'type: ignore' is a lot more complicated to do. See:https://github.com/psf/black/pull/2092\#issuecomment-817178166

JelleZijlstra · 2021-04-11T20:42:40Z

tests/data/comments_non_breaking_space.py

@@ -0,0 +1,271 @@
+from .config import (
+    Any,


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

Closes psf#2091

The switch to unvalid 'type: ignore' to valid 'type: ignore' is a lot more complicated to do. See:https://github.com/psf/black/pull/2092\#issuecomment-817178166

Also handle notmal comment not starting with 'type:' better.

JelleZijlstra

Thanks!

Pierre-Sassoulas · 2021-04-11T21:41:32Z

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 · 2021-04-11T21:42:31Z

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

Pierre-Sassoulas force-pushed the master branch 2 times, most recently from ee28570 to 145d46e Compare April 7, 2021 19:25

Pierre-Sassoulas mentioned this pull request Apr 10, 2021

Remove non-breaking space at the beggining of comments #2091

Closed

Pierre-Sassoulas referenced this pull request in Pierre-Sassoulas/black Apr 11, 2021

Leave non breaking space in type: ignore for security

4732453

See rational:https://github.com/psf/black/pull/2092\#issuecomment-817178166

Pierre-Sassoulas referenced this pull request in Pierre-Sassoulas/black Apr 11, 2021

Leave non breaking space in 'type: ignore' to be safe

3b7b0b7

See rational:https://github.com/psf/black/pull/2092\#issuecomment-817178166

Pierre-Sassoulas force-pushed the master branch from 4732453 to 3b7b0b7 Compare April 11, 2021 09:14

Pierre-Sassoulas referenced this pull request in Pierre-Sassoulas/black Apr 11, 2021

Leave non breaking space in 'type: ignore' to be safe

8e55788

See rational:https://github.com/psf/black/pull/2092\#issuecomment-817178166

Pierre-Sassoulas force-pushed the master branch from 3b7b0b7 to 8e55788 Compare April 11, 2021 09:17

Pierre-Sassoulas force-pushed the master branch from 8e55788 to f66a033 Compare April 11, 2021 09:37

JelleZijlstra reviewed Apr 11, 2021

View reviewed changes

src/black/__init__.py Outdated Show resolved Hide resolved

JelleZijlstra reviewed Apr 11, 2021

View reviewed changes

tests/data/comments7.py Outdated Show resolved Hide resolved

ichard26 reviewed Apr 11, 2021

View reviewed changes

Pierre-Sassoulas requested a review from JelleZijlstra April 11, 2021 19:33

JelleZijlstra requested changes Apr 11, 2021

View reviewed changes

Pierre-Sassoulas force-pushed the master branch from d3288db to 41d50de Compare April 11, 2021 20:15

Pierre-Sassoulas force-pushed the master branch from 41d50de to d7a633c Compare April 11, 2021 20:21

JelleZijlstra reviewed Apr 11, 2021

View reviewed changes

Pierre-Sassoulas added 3 commits April 11, 2021 23:33

Remove NBSP at the beggining of comments

8b85867

Closes psf#2091

Leave non breaking space in comment with 'type: ignore' inside

b3a0da7

The switch to unvalid 'type: ignore' to valid 'type: ignore' is a lot more complicated to do. See:https://github.com/psf/black/pull/2092\#issuecomment-817178166

Handle the case with type hint in the pre-3.6 style

08fe7f1

Also handle notmal comment not starting with 'type:' better.

Pierre-Sassoulas force-pushed the master branch from d7a633c to 08fe7f1 Compare April 11, 2021 21:34

JelleZijlstra approved these changes Apr 11, 2021

View reviewed changes

JelleZijlstra merged commit d960d5d into psf:master Apr 11, 2021

Pierre-Sassoulas mentioned this pull request Apr 11, 2021

Fix crash for non breaking spaces in docstring #2099

Closed

		NON_BREAKING_SPACE = " "
		if content and content[0] == NON_BREAKING_SPACE and "type: ignore" not in content:


		#### _Black_

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

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

Remove NBSP at the beggining of comments #2092

Remove NBSP at the beggining of comments #2092

Uh oh!

Conversation

Pierre-Sassoulas commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ichard26 commented Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JelleZijlstra commented Apr 10, 2021

Uh oh!

Pierre-Sassoulas commented Apr 11, 2021

Uh oh!

Uh oh!

Uh oh!

ichard26 Apr 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra left a comment

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas commented Apr 11, 2021

Uh oh!

JelleZijlstra commented Apr 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Pierre-Sassoulas commented Apr 7, 2021 •

edited

Loading

ichard26 commented Apr 10, 2021 •

edited

Loading

ichard26 Apr 11, 2021 •

edited

Loading