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

Hanging comments inside multi-line expressions are squeezed #77

Open
effigies opened this issue Jul 23, 2022 · 5 comments · May be fixed by #83
Open

Hanging comments inside multi-line expressions are squeezed #77

effigies opened this issue Jul 23, 2022 · 5 comments · May be fixed by #83

Comments

@effigies
Copy link
Contributor

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
Copy link
Collaborator

warsaw commented Jul 23, 2022

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Collaborator

warsaw commented Oct 7, 2023

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
Copy link
Owner

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.

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.

4 participants