psf / black

The uncompromising Python code formatter
https://black.readthedocs.io/en/stable/
MIT License
38.62k stars 2.43k forks source link

Numpy-style indexing gets "mushed" (i.e. loses a space) #33

Closed dhermes closed 6 years ago

dhermes commented 6 years ago

For the repro file its_science.py

def first_column1d(values):
    """First column, as a 1D array."""
    return values[:, 0]

def first_column2d(values):
    """First column, but still a 2D array."""
    return values[:, 0:1]

Running black (installed from HEAD as in #30)

$ venv-HEAD/bin/black its_science.py
reformatted its_science.py

changes the "exotic" slice argument:

diff --git a/its_science.py b/its_science.py
index f3c4a8b..3a5dba0 100644
--- a/its_science.py
+++ b/its_science.py
@@ -5,4 +5,4 @@ def first_column1d(values):

 def first_column2d(values):
     """First column, but still a 2D array."""
-    return values[:, 0:1]
+    return values[:,0:1]

PS @ambv This is my fourth and last issue (for now) and it's just "wrong" things I encountered when running this on a large codebase of mine. I am SOOOOO happy you have created black and look forward to evangelizing it in the community.

ambv commented 6 years ago

This is now fixed and will be part of 18.3a2. Thanks for all your reports!

dhermes commented 6 years ago

Awesome response time, you rock!

dhermes commented 6 years ago

@ambv It seems the fix didn't cover all cases. Given moar_numpy.py:

def delta(points):
    return points[:, 1:] - points[:, :-1]

running (from HEAD)

$ venv/bin/black moar_numpy.py
reformatted moar_numpy.py

the 2nd slice gets "mushed":

diff --git a/moar_numpy.py b/moar_numpy.py
index d9ba020..e36b529 100644
--- a/moar_numpy.py
+++ b/moar_numpy.py
@@ -1,2 +1,2 @@
 def delta(points):
-    return points[:, 1:] - points[:, :-1]
+    return points[:, 1:] - points[:,:-1]
ambv commented 6 years ago

Alright, will add a test with this example, too. Missed today's release but there should be another one in a few days tops :-)

ambv commented 6 years ago

This should solve your problem once and for all: https://github.com/ambv/black/commit/21c49d942264a1f36858036a9235e5ecb337bf4c

dhermes commented 6 years ago

Thanks for another quick fix! I just tried installed the latest

$ git log -1 --pretty=%H
21c49d942264a1f36858036a9235e5ecb337bf4c
$ venv/bin/pip install .

and re-ran it on my mostly converted codebase and it seems there is a regression in how slices are handled. Consider moar_numpy.py

def first_row1d(points):
    return points[0, :]

Running

$ venv/bin/black moar_numpy.py
reformatted moar_numpy.py

yields

diff --git a/moar_numpy.py b/moar_numpy.py
index 4a057d6..f1efb82 100644
--- a/moar_numpy.py
+++ b/moar_numpy.py
@@ -1,2 +1,2 @@
 def first_row1d(points):
-    return points[0, :]
+    return points[0,:]

I promise I'm not trying to be difficult :grinning:. I say regression because black used to leave those statements alone (I think?).

ambv commented 6 years ago

Sigh...

ambv commented 6 years ago

😂

ambv commented 6 years ago

Is this the last form? If you have any other uses of tuple indexing in your code, list them all please so I can add tests in one go.

dhermes commented 6 years ago

Sure thing. I did a git grep [ to find every use of slicing in the project I've been running black on and found the following comprehensive-ish list of all "slice types":

mat[0, 4]
mat[0, :]
mat[:, i]
mat[0, :2]
mat[:N, 0]
mat[:, 1:3]
mat[:2, :4]
mat[2:4, 1:5]
mat[4:, 2:]
mat[:, (0, 1, 2, 5)]
mat[0, [0]]
mat[:, [i]]
mat.flat[d::d + 1]
mat[:c, c - 1]
mat[1:c + 1, c]
mat[-(c + 1):, d]
mat[:, l[-2]]
mat[:, ::-1]
vec[np.newaxis, :]

I "normalized" the list. I'm sure there are lots more ways that people use slices in the wild. I also have other projects with 3D arrays, so slices like mat[:, :, j] are common.

dhermes commented 6 years ago

Thanks again!