grantjenks / blue

The slightly less uncompromising Python code formatter.
https://blue.readthedocs.io/
Other
395 stars 21 forks source link

Hanging comments inside multi-line expressions are squeezed #77

Open effigies opened 2 years ago

effigies commented 2 years ago

It would be nice to retain the whitespace following lines even in multi-line expressions, such as list or dict definitions. Here is a (truncated) example where we are defining named fields in a binary format:

 header_dtd = [
-    ('sizeof_hdr', 'i4'),      # 0; must be 348
-    ('data_type', 'S10'),      # 4; unused
-    ('db_name', 'S18'),        # 14; unused
-    ('extents', 'i4'),         # 32; unused
-    ('session_error', 'i2'),   # 36; unused
-    ('regular', 'S1'),         # 38; unused
-    ('dim_info', 'u1'),        # 39; MRI slice ordering code
-    ('dim', 'i2', (8,)),       # 40; data array dimensions
+    ('sizeof_hdr', 'i4'),  # 0; must be 348
+    ('data_type', 'S10'),  # 4; unused
+    ('db_name', 'S18'),  # 14; unused
+    ('extents', 'i4'),  # 32; unused
+    ('session_error', 'i2'),  # 36; unused
+    ('regular', 'S1'),  # 38; unused
+    ('dim_info', 'u1'),  # 39; MRI slice ordering code
+    ('dim', 'i2', (8,)),  # 40; data array dimensions
 ]

I realize this is going to be hard to get right 100% of the time, especially if other parts of the expression need to be shifted. I can just #fmt: off it, but would there be any interest in handling this case?

Using: blue, version 0.9.0, based on black 22.1.0

Follow-up to #20 and #31.

warsaw commented 2 years ago

I haven't confirmed it yet, but this definitely worked at one point. It's one of the things that distinguishes blue from black. If confirmed, it's a regression (possibly caused by a change in black?).

effigies commented 2 years ago

Installing blue 0.6.0 and black 20.8b1 (which should have been the active version at the time #31 was first released), this same thing happens, so I don't think it's a regression, just a use case that wasn't covered.

FWIW, this whitespace is still preserved:

x = 1        # comment1
y = 3.14159  # comment2
haxwithaxe commented 1 year ago

This is happening again in master in the tests. The repo dir is blue-haxwithaxe-fork because I just forked the repo to fix the flake8 coupling, but I haven't changed anything yet.

$ tox -s -e py310
.pkg: _optional_hooks> python /home/hax/dev/blue-haxwithaxe-fork/.venv/lib/python3.10/site-packages/pyproject_api/_backend.py True setuptools.build_meta __legacy__
.pkg: get_requires_for_build_sdist> python /home/hax/dev/blue-haxwithaxe-fork/.venv/lib/python3.10/site-packages/pyproject_api/_backend.py True setuptools.build_meta __legacy__
.pkg: get_requires_for_build_wheel> python /home/hax/dev/blue-haxwithaxe-fork/.venv/lib/python3.10/site-packages/pyproject_api/_backend.py True setuptools.build_meta __legacy__
.pkg: prepare_metadata_for_build_wheel> python /home/hax/dev/blue-haxwithaxe-fork/.venv/lib/python3.10/site-packages/pyproject_api/_backend.py True setuptools.build_meta __legacy__
.pkg: build_sdist> python /home/hax/dev/blue-haxwithaxe-fork/.venv/lib/python3.10/site-packages/pyproject_api/_backend.py True setuptools.build_meta __legacy__
py310: install_package> python -I -m pip install --force-reinstall --no-deps /home/hax/dev/blue-haxwithaxe-fork/.tox/.tmp/package/18/blue-0.9.1.tar.gz
py310: commands[0]> pytest
================================================= test session starts =================================================
platform linux -- Python 3.10.12, pytest-7.4.2, pluggy-1.3.0
cachedir: .tox/py310/.pytest_cache
rootdir: /home/hax/dev/blue-haxwithaxe-fork
configfile: tox.ini
testpaths: blue, docs, tests
plugins: cov-4.1.0
collected 8 items                                                                                                     

tests/test_blue.py ....F...                                                                                     [100%]

====================================================== FAILURES =======================================================
_____________________________________________ test_good_dirs[good_cases] ______________________________________________

monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7feda520a9b0>, test_dir = 'good_cases'

    @pytest.mark.parametrize(
        'test_dir',
        [
            'config_setup',
            'config_tox',
            'config_blue',
            'config_pyproject',
            'good_cases',
        ],
    )
    def test_good_dirs(monkeypatch, test_dir):
        src_dir = tests_dir / test_dir
        monkeypatch.setattr('sys.argv', ['blue', '--check', '--diff', '.'])
        with TemporaryDirectory() as dst_dir:
            # warsaw(2022-05-01): we can't use shutil.copytree() here until we
            # drop Python 3.7 support because we need dirs_exist_ok and that was
            # added in Python 3.8
            for path in src_dir.rglob('*'):
                copy(src_dir / path, dst_dir)
            monkeypatch.chdir(dst_dir)
            black.find_project_root.cache_clear()
            with pytest.raises(SystemExit) as exc_info:
                asyncio.set_event_loop(asyncio.new_event_loop())
                blue.main()
            # warsaw(2022-05-02): Change back to the srcdir now so that when the
            # context manager exits, the dst_dir can be properly deleted.  On
            # Windows, that will fail if the process's cwd is still dst_dir.
            # Python 3.11 has a contextlib.chdir() function we can use eventually.
            monkeypatch.chdir(src_dir)
>           assert exc_info.value.code == 0
E           assert 1 == 0
E            +  where 1 = SystemExit(1).code
E            +    where SystemExit(1) = <ExceptionInfo SystemExit(1) tblen=4>.value

/home/hax/dev/blue-haxwithaxe-fork/tests/test_blue.py:46: AssertionError
------------------------------------------------ Captured stdout call -------------------------------------------------
--- hanging_comments.py 2023-10-07 16:30:00.117352 +0000
+++ hanging_comments.py 2023-10-07 16:30:00.181770 +0000
@@ -1,9 +1,9 @@
 a = 1    # Hanging
 b = 22   # Comments
 c = 333  # Are fine

 nested_arr = [
-    [1, 4, 7],      # These
-    [10, 13, 16],   # Should be
-    [19, 22, 25],   # Too
+    [1, 4, 7],  # These
+    [10, 13, 16],  # Should be
+    [19, 22, 25],  # Too
 ]
------------------------------------------------ Captured stderr call -------------------------------------------------
would reformat hanging_comments.py

Oh no! 💥 💔 💥
1 file would be reformatted, 3 files would be left unchanged.

pip freeze inside the .tox/py310 directory

$ pip freeze
black==22.1.0
blue @ file:///home/hax/dev/blue-haxwithaxe-fork/.tox/.tmp/package/18/blue-0.9.1.tar.gz#sha256=7351a7fd05fa378f88764d7f1ca01a0ce780168af3ec0ec9ef9d31d2a6e9602a
click==8.1.7
coverage==7.3.2
exceptiongroup==1.1.3
flake8==4.0.1
iniconfig==2.0.0
mccabe==0.6.1
mypy-extensions==1.0.0
packaging==23.2
pathspec==0.11.2
platformdirs==3.11.0
pluggy==1.3.0
pycodestyle==2.8.0
pyflakes==2.4.0
pytest==7.4.2
pytest-cov==4.1.0
tomli==2.0.1
warsaw commented 1 year ago

I don't know about @grantjenks but now that ruff's formatter is in alpha and it handles single quotes vs double quotes via configuration, I'm personally highly unmotivated to put much work into this (not that I had a lot of bandwidth anyway, but ruff is that good).

However, hanging comments are still under discussion and I am trying to encourage the ruff maintainers to at least support blue's style as an option. Please go over to that ticket and weigh in!

grantjenks commented 1 year ago

I just took ruff for a test drive and first impression -- I agree. I'd like to see it exit alpha but it's very promising.