Revise isort config

[ruestefa]

Adapt and improve config of isort (which groups and sorts imports) in pyproject.toml.

The examples below have been tested with Python 3.10.8, isort 5.12.0 and black 22.10.0.

Step 1: Black profile

Use the (newish) "black" profile option, which replaces some previously set options and notably implies the previously unset include_trailing_comma (which might change existing code slightly).

Old:

...
[tool.isort]
default_section = "THIRDPARTY"
force_grid_wrap = false
force_single_line = true
line_length = 88
order_by_type = false
# Headings
...

New:

...
[tool.isort]
default_section = "THIRDPARTY"
profile = "black"
force_single_line = true
order_by_type = false
# Headings
...

Step 2: NOQA

In one project, I stumbled upon a problematic case with imports that required multiple directive comments that caused the lines to be too long. Complete example:

"""Test isort config."""
# pylint: disable=import-error

# Third-party
import numpy as np
from metpy.interpolate import interpolate_1d  # type: ignore [import]
from netCDF4 import Dataset  # type: ignore [import]  # pylint: disable=no-name-in-module
from netCDF4 import Dataset as open_ncfile  # pylint: disable=reimported,no-name-in-module
from numpy.fft import fft2

# pylint: disable-next=pointless-statement
np, interpolate_1d, Dataset, open_ncfile, fft2

The setup above with profile = "black" etc. produces this after running isort:

"""Test isort config."""
# pylint: disable=import-error

# Third-party
import numpy as np
from metpy.interpolate import interpolate_1d  # type: ignore [import]
from netCDF4 import (
    Dataset,  # type: ignore [import]  # pylint: disable=no-name-in-module
)
from netCDF4 import Dataset as open_ncfile  # pylint: disable=reimported,no-name-in-module
from numpy.fft import fft2

# pylint: disable-next=pointless-statement
np, interpolate_1d, Dataset, open_ncfile, fft2

and this after additionally running black:

"""Test isort config."""
# pylint: disable=import-error

# Third-party
import numpy as np
from metpy.interpolate import interpolate_1d  # type: ignore [import]
from netCDF4 import (
    Dataset,  # type: ignore [import]  # pylint: disable=no-name-in-module
)
from netCDF4 import (
    Dataset as open_ncfile,
)  # pylint: disable=reimported,no-name-in-module
from numpy.fft import fft2

# pylint: disable-next=pointless-statement
np, interpolate_1d, Dataset, open_ncfile, fft2

Neither versions walidates with pylint because of misplaced # pylint: disable directives (which is ultimately a bug -- or at least a missing feature -- in black; there are open issues about this). The problem is that placing comments correctly is nontrivial, especially when directives of different tools with different placement/scoping rules are involved.

Luckily, isort provides the option multi_line_output=7, where 7 stands for 7-noqa, which doesn't change too long lines but simply adds a # NOQA directive (must be uppercase...) if there is none already, which tells some linters (like flake8) to ignore the line:

...
[tool.isort]
default_section = "THIRDPARTY"
profile = "black"
force_single_line = true
order_by_type = false
multi_line_output = 7  # 7-noqa
# Headings
...

Running isort now yields:

"""Test isort config."""
# pylint: disable=import-error

# Third-party
import numpy as np
from metpy.interpolate import interpolate_1d  # type: ignore [import]
from netCDF4 import Dataset  # type: ignore [import]  # pylint: disable=no-name-in-module  # NOQA
from netCDF4 import Dataset as open_ncfile  # pylint: disable=reimported,no-name-in-module
from numpy.fft import fft2

# pylint: disable-next=pointless-statement
np, interpolate_1d, Dataset, open_ncfile, fft2

Unfortunately, a bug in isort causes # NOQA not to be added to lines with as imports (I've opened an issue), so we'll have to add it manually. Furthermore, while black doesn't expand the first netCDF4 line because the trailing comment starts with # type: ignore (see here), it still expands the next line, thereby still misplacing the # pylint directive. We thus need to manually disable black for this line by wrapping it in # fmt: off and # fmt: on:

"""Test isort config."""
# pylint: disable=import-error

# Third-party
# fmt: off
import numpy as np
from metpy.interpolate import interpolate_1d  # type: ignore [import]
from numpy.fft import fft2
from netCDF4 import Dataset  # type: ignore [import]  # pylint: disable=no-name-in-module
from netCDF4 import Dataset as open_ncfile  # pylint: disable=reimported,no-name-in-module  # NOQA
# fmt: on

# pylint: disable-next=pointless-statement
np, interpolate_1d, Dataset, open_ncfile, fft2

(Note that while black now supports the inline directive fmt: skip to skip single line, it's unfortunately still buggy; see multiple open issues such as this. Also note that we wrap the whole "Third-party" block in fmt: off/on to make sure that isort does not misplace the directives while sorting the imports because it considers comments to belong to the next import line.)

Running isort over this adapted code with the new config yields:

"""Test isort config."""
# pylint: disable=import-error

# Third-party
# fmt: off
import numpy as np
from metpy.interpolate import interpolate_1d  # type: ignore [import]
from netCDF4 import Dataset  # type: ignore [import]  # pylint: disable=no-name-in-module  # NOQA
from netCDF4 import Dataset as open_ncfile  # pylint: disable=reimported,no-name-in-module  # NOQA
from numpy.fft import fft2

# fmt: on

# pylint: disable-next=pointless-statement
np, interpolate_1d, Dataset, open_ncfile, fft2

which validates with both flake8 and pylint and which black doesn't change any further.

The final proposed isort config with a rather detailed comment:

...
[tool.isort]
default_section = "THIRDPARTY"
profile = "black"
force_single_line = true
order_by_type = false
# Set `multi_line_output = 7` to mark too long lines with `# NOQA` (uppercase!)
# instead of spreading them over multiple lines. This leaves lines with long
# trailing comments intact, which may occur if multiple directives are necessary
# (e.g., `# type: ignore [import]  # pylint: disable=no-name-in-module`).
# (Note that thanks to `force_single_line = true`, this should be the main
# reason for too long lines because multi-import lines are already broken up.)
multi_line_output = 7  # 7=noqa
honor_noqa = false
# Headings
...

[ruestefa]

I've taken the opportunity to remove the configs of Python-specific tools from the blueprints root pyproject.toml, since it doesn't deal with Python code directly (that is done in projects created from the blueprint). The only linters invoked at the blueprint root level with config in pyproject.toml are rstcheck and codespell.

[clairemerker]

I didn't test it to be honest but It looks good to me :)

[ruestefa]

I'll apply this to our repos today or tomorrow, then I can give some feedback on whether this causes frequent code changes. I don't expect so since with force_single_line=true, the implicit change to include_trailing_comma shouldn't have any effect, and super-long trailing comments are likely not too frequent, but let's see.

[ruestefa]

I'll apply this to our repos today or tomorrow, then I can give some feedback on whether this causes frequent code changes.

Forgot to give feedback here; all good, no unintended consequences. Ready to merge as far as I'm concerned.

[clairemerker]

Perfect, thanks a lot! You can merge anytine :)