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

Unexpected incompatibility with reorder-python-imports #4175

Closed
maxwell-k opened this issue Jan 26, 2024 · 21 comments
Closed

Unexpected incompatibility with reorder-python-imports #4175

maxwell-k opened this issue Jan 26, 2024 · 21 comments
Labels
T: bug Something isn't working

Comments

@maxwell-k
Copy link

maxwell-k commented Jan 26, 2024

Describe the bug

It is impossible to use Black 24.1.0 alongside reorder-python-imports. Using the example from #1872 , reorder-python-imports produces:

"""Module docstring."""
import typing 

While formatting with black and default arguments produces the following:

"""Module docstring."""

import typing

This new behaviour makes the two tools incompatible. The author of reorder-python-imports suggests this is an issue with Black.

Is it possible to change Black to work better with reorder-python-imports please? I have used both tools together for a long time.

Environment

black, 24.1.0 (compiled: yes)
Python (CPython) 3.12.1

Also checked on the online formatter.

Additional context

A little bit of context from the Black changelog is that Black 24 and above will:

Enforce newline after module docstrings (#3932, #4028)

1872 was the original Black issue, and changes were implemented in 3932; 4028 doesn't relate to imports.

@maxwell-k maxwell-k added the T: bug Something isn't working label Jan 26, 2024
@maxwell-k maxwell-k changed the title Unexpected incompatibility between Black and reorder-python-imports Unexpected incompatibility with reorder-python-imports Jan 26, 2024
@JelleZijlstra
Copy link
Collaborator

It's unfortunate that nobody reported this while the feature was in the preview style and in the alphas. Our guarantee is that we'll keep the style the same for the rest of the year, so we can't change it now.

I think it should be Black's job to decide on whitespace outside the import block, not reorder-python-imports's. Unfortunately that means I don't have a good resolution for you.

@cooperlees
Copy link
Collaborator

I'll second Jelle here and state that an import re-ordering tool should respect the syntax it was given when it started it's run.

Here, if there is white space between an import and a docstring, it should be maintained. It's not a formatting tool, the white space has nothing to do with fixing your imports, so I feel this tool is going outside it's scope.

@maxwell-k
Copy link
Author

I agree this is unfortunate. I really like the other changes in the current style that was adopted with 24.1.0. I didn't know about the guarantee - I do now . I hadn't thought to test the preview style or alphas; probably because I didn't realise this is an uncommon combination of tools. Again I've learnt something, thank you.

I'm sorry if this issue is not the right place for this conversation; the original report is closed, locked as resolved and I'm unable to comment.

For the code I'm working with today # fmt: skip after the closing """ on the problem docstrings is an acceptable workaround.

Reading using black with other tools again; I suggest a simple solution might be to document that these two tools are not compatible, that what I'm trying to do is unsupported.

I think it should be Black's job to decide on whitespace outside the import block, not reorder-python-imports's. Unfortunately that means I don't have a good resolution for you.

Here, if there is white space between an import and a docstring, it should be maintained. It's not a formatting tool, the white space has nothing to do with fixing your imports, so I feel this tool is going outside it's scope.

To get to an important point, now I have seen:

  1. the maintainer of the import reordering tool explain that the issue is with the code formatting tool and
  2. the maintainers of the code formatting tool explain that the problem is with the import reordering tool

I don't want to comment either way. Today, the two tools are incompatible. If future versions are to be compatible one or both must change. The import reordering tool appears to have ruled out changes. Are changes a possibility for this code formatting tool?

Thank you for you help! Black is a great piece of software.

@JelleZijlstra
Copy link
Collaborator

I realize you're in an unfortunate situation where two opinionated tools disagree in an incompatible way, and I wish we'd have known about it during the preview period. I won't rule out changing our style here in the future if there's a good reason to do so, but I think that's unlikely. It's definitely within Black's purview to control whitespace after the module docstring.

Possible workarounds:

  • In your CI, ensure that your code is clean if you use reorder-python-imports then Black (or the other way around), instead of running only one of the tools.
  • Use a different tool for import sorting, e.g. isort, usort, or Ruff's equivalent of isort.

@maxwell-k
Copy link
Author

maxwell-k commented Jan 26, 2024

The only reason to change that I am aware of is to make Black compatible with reorder-python-imports again. Is that a good reason or not? I'll let you decide.

I agree both are opinionated tools with a specific style. I should perhaps make it explicit here that the style implemented by reorder-python-imports has "a single aim: reduce merge conflicts" with a documented rationale. A short example is:

-from typing import Dict, List
+from typing import Dict
+from typing import List

The other functionality in reorder-python-imports is also available in other import sorting tools. I looked at isort and ruff; usort is new to me, thank you for mentioning it. I am not aware of a tool other than reorder-python-imports that implements this "reduce merge conflicts" import style.

isort is the only documented compatible import ordering tool for black. Does that imply anything about future compatibilty?

The ignore comments ( # fmt: skip after the closing """ ) I mentioned above are OK for a short term workaround. Longer term, if neither tool is going to change, you're right I'll need to carefully reconsider my tool choices.

Edited to add: thanks again for dealing with this unfortunate incompatibility carefully. I appreciate your help.

@JelleZijlstra
Copy link
Collaborator

Might be worth asking Ruff to implement the one-import-per-line style as an option. (I searched their issue tracker but didn't find an existing issue asking for it.)

@zachborboacryptofi
Copy link

maybe just use isort with something like:

[tool.isort]
profile = "black"
force_single_line = "true"

nicoddemus added a commit to nicoddemus/pytest-mock that referenced this issue Jan 29, 2024
Unfortunately black and reorder-python-imports are no longer compatible between each other:

asottile/reorder-python-imports#367
asottile/reorder-python-imports#366
psf/black#4175

Take this opportunity to try out ruff.
nicoddemus added a commit to nicoddemus/execnet that referenced this issue Jan 29, 2024
Unfortunately black and reorder-python-imports are no longer compatible between each other:

asottile/reorder-python-imports#367
asottile/reorder-python-imports#366
psf/black#4175

Take this opportunity to try out ruff.
nicoddemus added a commit to nicoddemus/execnet that referenced this issue Jan 29, 2024
Unfortunately black and reorder-python-imports are no longer compatible between each other:

asottile/reorder-python-imports#367
asottile/reorder-python-imports#366
psf/black#4175

Take this opportunity to try out ruff.
nicoddemus added a commit to ESSS/barril that referenced this issue Jan 30, 2024
Unfortunately `black` and `reorder-python-imports` are no longer compatible between each other, and will not be in the foreseeable feature:

asottile/reorder-python-imports#367
asottile/reorder-python-imports#366
psf/black#4175

Drop `reorder-python-imports` in favor of `isort`.
nicoddemus added a commit to ESSS/oop-ext that referenced this issue Jan 30, 2024
Unfortunately `black` and `reorder-python-imports` are no longer compatible between each other, and will not be in the foreseeable feature:

asottile/reorder-python-imports#367
asottile/reorder-python-imports#366
psf/black#4175

Drop `reorder-python-imports` in favor of `isort`.
nicoddemus added a commit to pytest-dev/execnet that referenced this issue Jan 30, 2024
Unfortunately black and reorder-python-imports are no longer compatible between each other:

asottile/reorder-python-imports#367
asottile/reorder-python-imports#366
psf/black#4175

Take this opportunity to try out ruff.

Closes #231
nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jan 30, 2024
Unfortunately black and reorder-python-imports are no longer compatible, and from the looks of it probably will not be compatible anytime soon:

asottile/reorder-python-imports#367
asottile/reorder-python-imports#366
psf/black#4175

This replaces `reorder-python-imports` by `isort` configured in a way to yield roughtly the same results.

Closes pytest-dev#11885
@adamchainz
Copy link
Contributor

adamchainz commented Feb 3, 2024

🤷 This is a real shame, I really like the speed and low-configurability of reorder-python-imports. I do also think its rearrangement of non-import newlines is outside its wheelhouse.

FWIW, below is the small patch that makes reorder-python-imports compatible. Debating whether it's worth forking to make an “always Black-compatible” version...

diff --git reorder_python_imports.py reorder_python_imports.py
index d66dc4b..3de766f 100644
--- reorder_python_imports.py
+++ reorder_python_imports.py
@@ -94,10 +94,10 @@ def partition_source(src: str) -> tuple[str, list[str], str, str]:
             pre_import = False
             chunks.append((CodeType.IMPORT, s))
         elif token_type is Tok.NEWLINE:
-            if s.isspace():
-                tp = CodeType.NON_CODE
-            elif pre_import:
+            if pre_import:
                 tp = CodeType.PRE_IMPORT_CODE
+            elif s.isspace():
+                tp = CodeType.NON_CODE
             else:
                 tp = CodeType.CODE
 
diff --git tests/reorder_python_imports_test.py tests/reorder_python_imports_test.py
index c61c39a..4db72fd 100644
--- tests/reorder_python_imports_test.py
+++ tests/reorder_python_imports_test.py
@@ -47,7 +47,7 @@ def test_tokenize_can_match_strings(s):
 @pytest.mark.parametrize(
     's',
     (
-        pytest.param('', id='trivial'),
+        pytest.param('\n', id='trivial'),
         pytest.param('#!/usr/bin/env python\n', id='shebang'),
         pytest.param('# -*- coding: UTF-8 -*-\n', id='source encoding'),
         pytest.param('  # coding: UTF-8\n', id='source encoding indented'),
@@ -190,17 +190,21 @@ def test_partition_source_imports_only(s, expected):
     assert nl == '\n'
 
 
-def test_partition_source_before_removes_newlines():
+def test_partition_source_before_leaves_newlines():
     before, imports, after, nl = partition_source(
         '# comment here\n'
         '\n'
-        '# another comment here\n',
+        '# another comment here\n'
+        '\n'
+        'import os\n'
     )
     assert before == (
         '# comment here\n'
+        '\n'
         '# another comment here\n'
+        '\n'
     )
-    assert imports == []
+    assert imports == ['import os\n']
     assert after == ''
     assert nl == '\n'
 

@tolomea
Copy link

tolomea commented Feb 4, 2024

The guy who maintains reorder-python-imports is very "my way or the highway", I suggest you take that advice and use isort or the fork suggestion @adamchainz made.
The place I work already dropped reorder-python-imports not because it didn't work for us but specifically because of his attitude.
If one of you does fork it please let me know, we would definitely consider switching to an import sorter that had the same sort of considered opination that Black has.

agittins added a commit to agittins/bermuda that referenced this issue Feb 28, 2024
- refer psf/black#4175
- issue identified by jaymunro #114
- bumped pre-commit-hooks repo to v4.5.0
agittins added a commit to agittins/bermuda that referenced this issue Feb 28, 2024
* wip: Trying to get passive updates

- nothing functional yet, but since nothing breaks either, committing it for now.
- plan is to allow updates to be triggered by incoming advertisements from devices we are maintaining sensors for.

* chore: Move from reorder-imports to isort

- refer psf/black#4175
- issue identified by jaymunro #114
- bumped pre-commit-hooks repo to v4.5.0

* chore: update black and apply formattings

- disabled check on const line because back-and-forth linting conflicts should not take more than three hours to solve.

- added black to requirements and bumped to same version in github workflow

- gitignore node_modules/.cache for new prettier version.
waynesun09 pushed a commit to RedHatQE/pylero that referenced this issue Mar 5, 2024
lemon24 added a commit to lemon24/reader that referenced this issue Mar 10, 2024
@Achimh3011
Copy link

Hmm, black makes a change so it is incompatible with python-reorder-importsand tells us "tough luck" for those who use both.

I'll move on to ruff.

@nicoddemus
Copy link
Contributor

I suggest for this issue to be closed, with the solution being using isort: #4175 (comment)

Perhaps it can be pinned for some time so others can find it quickly. 👍

@pbhuss
Copy link

pbhuss commented Mar 10, 2024

Hmm, black makes a change so it is incompatible with python-reorder-importsand tells us "tough luck" for those who use both.

I'll move on to ruff.

@Achimh3011 ruff won't help you here either -- they've adopted the same incompatible rule into their 2024.2 style
astral-sh/ruff#7995

@Achimh3011
Copy link

Unfortunately, you're right. I wanted to leave the quarrels behind by moving to a single tool, but the missing single imports feature of the import sorter of ruff spoils it for me. Either I stick to an older version of black or deactivate automatic code formatting.

I have no opinion on the newline itself, it's only the incompatibility that causes the pain.

@zstyblik
Copy link

zstyblik commented Apr 4, 2024

Perhaps this is a good place to ask, because I have exactly the same problem Black Vs. python-reorder-imports. Is it possible to somehow disable Enforce newline after module docstrings? I went through related GH issues, but I haven't found any way to do so - and perhaps there isn't one.

Thanks

@JelleZijlstra
Copy link
Collaborator

It is not possible.

@zstyblik
Copy link

zstyblik commented Apr 4, 2024

@JelleZijlstra understood :\ Thanks.

@JelleZijlstra
Copy link
Collaborator

JelleZijlstra commented Apr 4, 2024

Going to close this as we are not going to make any change to Black.

Summary:

@JelleZijlstra JelleZijlstra closed this as not planned Won't fix, can't repro, duplicate, stale Apr 4, 2024
@maxwell-k
Copy link
Author

Thank you for dealing with this issue so patiently @JelleZijlstra !

I've learnt from all the helpful comments; and I'm happy even if in the end the two tools are incompatible in the end.

I agree completely with your summary, one very minor point is that the third link "Adding a comment" should probably be this coment: #4175 (comment)

Thanks again.

yujinyuz added a commit to yujinyuz/django-as-a-package-layout-template that referenced this issue Apr 6, 2024
I'm glad that I discovered this issue sooner.

I really liked reorder-python-imports but it was impossible to make it
work with ruff and/or black.

The one small issue I had was ruff was adding a new line below a
docstring. While reorder-python-imports removes the new line.

For context:

- asottile/reorder-python-imports#370
- asottile/reorder-python-imports#366
- psf/black#4175
miguelinux added a commit to miguelinux/reorder-python-imports that referenced this issue Apr 14, 2024
This commits comes from

  * asottile#370
  * psf/black#4175 (comment)

because

  * asottile#367
  * asottile#366
  * psf/black#4175

Suggested-by: Achim Herwig <achim.herwig@wodca.de>
lkubb added a commit to lkubb/saltext-copier that referenced this issue Apr 28, 2024
black >=24 is incompatible with it since it adds a whitespace between
an initial docstring and the import section, which
reorder-python-imports does not like. This is something that is not
likely to be fixed. For reference:

asottile/reorder-python-imports#366
psf/black#4175

This substitution causes some reordering since isort separates
first-party imports from third-party ones by default. We're still
keeping the one line per import format to reduce merge conflicts.
@wimglenn
Copy link

If one of you does fork it please let me know, we would definitely consider switching to an import sorter that had the same sort of considered opination that Black has.

Reluctantly, I went ahead and made the fork

Hopefully in the future these projects can reconcile their differences and the fork can cease to exist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: bug Something isn't working
Projects
None yet
Development

No branches or pull requests