feat(pre-commit): add isort

[drts01]

Adding isort to have a consistent style for imports.

[tekktrik]

@kattni I really like isort and think it's worth adding here and patching to the libraries. I don't think it will mess with the try/except block we using for type annotation imports, but I can verify.

[jepler]

I like isort too! maybe there's some lib we can test it on to make sure the try/import/except blocks aren't affected? do we need to turn off any of the import-related pylint checks too, or does pylint fully endorse the changes that might be made by isort?

  wrong-import-order (C0411)
  ungrouped-imports (C0412)
  wrong-import-position (C0413)

[tekktrik]

If I recall correctly, isort typically doesn't conflict with black or pylint. The only one that I'm not fully sure about is ungrouped-imports, but off the top of my head I think it's fine.

[jepler]

I went ahead and ran isort across the bundle and found at least one problem. Note: finding one problem, or a small number of problems, shouldn't stop us from doing this. We just need to be aware of the scope of the problem before opting to do it.

It's the usual "pylint comment gets moved to where it doesn't have the desired effeect" problem that happens at the intersection of pylint & source formatters:

diff --git a/adafruit_24lc32.py b/adafruit_24lc32.py
index 8545f73..3fc44c7 100644
--- a/adafruit_24lc32.py
+++ b/adafruit_24lc32.py
@@ -29,12 +29,14 @@ Implementation Notes
 
 # imports
 import time
+
 from micropython import const
 
 try:
-    from typing import Optional, Union, Sequence
-    from digitalio import DigitalInOut
+    from typing import Optional, Sequence, Union
+
     from busio import I2C
+    from digitalio import DigitalInOut
 except ImportError:
     pass
 
@@ -233,8 +235,8 @@ class EEPROM_I2C(EEPROM):
         write_protect: bool = False,
         wp_pin: Optional[DigitalInOut] = None,
     ) -> None:
-        from adafruit_bus_device.i2c_device import (  # pylint: disable=import-outside-toplevel
-            I2CDevice as i2cdev,
+        from adafruit_bus_device.i2c_device import (
+            I2CDevice as i2cdev,  # pylint: disable=import-outside-toplevel
         )
 
         self._i2c = i2cdev(i2c_bus, address)

now, pylint produces the import-outside-toplevlel diagnostic, as the comment is not properly placed.

It also introduced pylint vs isort conflicts in at least one package: When black is run with the proper working directory, that doesn't happen, so it's fine.

************* Module amg88xx_rpi_thermal_cam
examples/amg88xx_rpi_thermal_cam.py:12:0: C0411: third party import "import board" should be placed before "import adafruit_amg88xx" (wrong-import-order)
examples/amg88xx_rpi_thermal_cam.py:13:0: C0411: third party import "import busio" should be placed before "import adafruit_amg88xx" (wrong-import-order)
examples/amg88xx_rpi_thermal_cam.py:14:0: C0411: third party import "import numpy as np" should be placed before "import adafruit_amg88xx" (wrong-import-order)
examples/amg88xx_rpi_thermal_cam.py:15:0: C0411: third party import "import pygame" should be placed before "import adafruit_amg88xx" (wrong-import-order)
examples/amg88xx_rpi_thermal_cam.py:16:0: C0411: third party import "from colour import Color" should be placed before "import adafruit_amg88xx" (wrong-import-order)
examples/amg88xx_rpi_thermal_cam.py:17:0: C0411: third party import "from scipy.interpolate import griddata" should be placed before "import adafruit_amg88xx" (wrong-import-order)
************* Module amg88xx_rpi_thermal_cam_console
examples/amg88xx_rpi_thermal_cam_console.py:12:0: C0411: third party import "import board" should be placed before "import adafruit_amg88xx" (wrong-import-order)
examples/amg88xx_rpi_thermal_cam_console.py:13:0: C0411: third party import "import busio" should be placed before "import adafruit_amg88xx" (wrong-import-order)
examples/amg88xx_rpi_thermal_cam_console.py:14:0: C0411: third party import "import numpy as np" should be placed before "import adafruit_amg88xx" (wrong-import-order)
examples/amg88xx_rpi_thermal_cam_console.py:15:0: C0411: third party import "from colour import Color" should be placed before "import adafruit_amg88xx" (wrong-import-order)
examples/amg88xx_rpi_thermal_cam_console.py:16:0: C0411: third party import "from scipy.interpolate import griddata" should be placed before "import adafruit_amg88xx" (wrong-import-order)
************* Module amg88xx_simpletest
examples/amg88xx_simpletest.py:7:0: C0411: third party import "import board" should be placed before "import adafruit_amg88xx" (wrong-import-order)
examples/amg88xx_simpletest.py:8:0: C0411: third party import "import busio" should be placed before "import adafruit_amg88xx" (wrong-import-order)

Note that I did not run black from pre-commit, just from the shell. However, I did use the 'black' profile:

find .  -name \*.py -print -exec isort --profile=black  {} +

[jepler]

about 50 "+" lines in the resulting diff across the bundle also contain "pylint" so that probably is approximately the scope of what will need human attention. This is in 28 separate repos.

[tekktrik]

@jepler Awesome, thank you!! That's totally patchable. Can you provide that list of repos that will need manual patching? I have a tool that will catch repos that fail the CI as well so I'll still run that, but good to have a tentative list.

[jepler]

most libs have a change introduced by isort. That wasn't what I concentrated on, because running pre-commit alone would fix the problem.

The following have a change affecting a line that has a pylint comment, and are more likely to need additional work beyond running pre-commit with isort enabled:

bundle/libraries/drivers/24lc32
bundle/libraries/drivers/amg88xx
bundle/libraries/drivers/bme280
bundle/libraries/drivers/bme680
bundle/libraries/drivers/bmp280
bundle/libraries/drivers/circuitplayground
bundle/libraries/drivers/crickit
bundle/libraries/drivers/epd
bundle/libraries/drivers/fram
bundle/libraries/drivers/gps
bundle/libraries/drivers/is31fl3741
bundle/libraries/drivers/l3gd20
bundle/libraries/drivers/lis3dh
bundle/libraries/drivers/ra8875
bundle/libraries/drivers/rgb-display
bundle/libraries/helpers/ble-broadcastnet
bundle/libraries/helpers/datetime
bundle/libraries/helpers/displayio_layout
bundle/libraries/helpers/led-animation
bundle/libraries/helpers/midi
bundle/libraries/helpers/motorkit
bundle/libraries/helpers/requests
bundle/libraries/helpers/rsa

[drts01]

I feel that my PR has generated a lot of work. Is there anything I can do to help?

[tekktrik]

I'm happy with this, and would be willing to help patch up things if we merge this. @jepler, if you want to merge and regenerate that list, I'll happily take care of the cleanup!

[jepler]

I think the cookiecutter change can be merged without immediate additional work, and it'd cover new libraries. It's just that when updating pre-commit later via adabot patch it'll require individual attention.

[jepler]

oh, wait, when running isort I think we want the "black profile"

- repo: https://github.com/pycqa/isort
  rev: 5.12.0
  hooks:
    - id: isort
      name: isort (python)
      args: ['--profile', 'black']

or similar

[jepler]

and pylint wrong-input-order does seem to need to be disabled globally

[justmobilize]

I would love to add isort, and happy to help update any/all libraries. The correct way at this point is:

  - repo: https://github.com/PyCQA/isort
    rev: 5.13.2
    hooks:
      - id: isort
        args: ["--profile", "black", "--filter-files"]

Happy to create a new PR if desired.

[justmobilize]

@jepler did you have an example on where wrong-input-order would get raised? We might be able to update isort to put things in the right spot...

[jepler]

No, I don't have that context from last year anymore, sorry.

[dhalbert]

We are updating cookie-cutter to use ruff (#237) so this will need updating before merging.

[justmobilize]

Since we are using:

[lint]
select = ["I", "PL", "UP"]

isort is included and this wouldn't be needed anymore, would it?

[FoamyGuy]

isort is included and this wouldn't be needed anymore, would it?

That matches my understanding.

As I understand it the ruff configuration will handle the sorting of imports for us. I believe this PR could be closed.

[dhalbert]

Thanks for the ruff info. I've closed.