pre-commit: adopt isort hook

[bjlittle]

This PR adds support for the isort pre-commit git hook to enforce an opinionated import sort order throughout the code-base.

The benefit is that there is one less distraction to consider for devs when reviewing pull-requests. Win 🥳

Reference: #1934

[bjlittle]

@dopplershift Thanks for the merges 👍 I'll rebase the other outstanding PRs asap... I'm afk atm

[bjlittle]

pre-commit.ci autofix

[bjlittle]

@dopplershift Conflicts resolved 👍

[greglucas]

Is it perhaps worth being consistent with MPL and their configuration? They recently changed over to isort for some of their examples: matplotlib/matplotlib#25099

[bjlittle]

Is it perhaps worth being consistent with MPL and their configuration? They recently changed over to isort for some of their examples: matplotlib/matplotlib#25099

@greglucas They appear to be appyling isort with the "out-the-box" default configuration, but only to their examples and tutorials, and not the rest of the code-base 😕

[greglucas]

Here is the configuration:
https://github.com/matplotlib/matplotlib/blob/43c35e7c51073ecf8f82789f5f42b3238117cb56/pyproject.toml#L10-L16

[bjlittle]

So, do you guys want to adopt that?

The custom section for pydata might be attractive, if you wanted to group such imports together? i.e., numpy and matplotlib

Otherwise, our configuration is about the same.

The no_lines_before = "LOCALFOLDER" pattern may address @QuLogic's comment 🤔

[bjlittle]

So, do you guys want to adopt that?

The custom section for pydata might be attractive, if you wanted to group such imports together? i.e., numpy and matplotlib

Otherwise, our configuration is about the same.

The no_lines_before = "LOCALFOLDER" pattern may address @QuLogic's comment thinking

Bingo! Yup, it certainly does. So it's possible to have isort give us:

Option 1

import cartopy.crs as ccrs

from .helpers import check_proj_params

Option 2

import cartopy.crs as ccrs
from .helpers import check_proj_params

Option 3

from .helpers import check_proj_params

import cartopy.crs as ccrs

Option 4

from .helpers import check_proj_params
import cartopy.crs as ccrs

Which pattern do you guys want to adopt?

Option 1 is the isort default.

@QuLogic was suggesting Option 2 (but this could also be Option 4, now we know the pattern to apply)

[rcomer]

I think this is the reasoning for the pydata section in mpl matplotlib/matplotlib#25099 (comment)

[QuLogic]

Which pattern do you guys want to adopt?

I don't have a strong opinion here; it was also mostly curiosity. I'd slightly lean towards 1 or 2, so maybe just leave it at the default?

[greglucas]

What about switching all of those from relative imports to explicit?
from cartopy.tests.crs.helpers import check_proj_params

[bjlittle]

Which pattern do you guys want to adopt?

I don't have a strong opinion here; it was also mostly curiosity. I'd slightly lean towards 1 or 2, so maybe just leave it at the default?

Cool, okay. Let's minimise the isort customisation and go with 1 as the default behaviour, which seem sensible enough 👍

[bjlittle]

What about switching all of those from relative imports to explicit? from cartopy.tests.crs.helpers import check_proj_params

I kinda like the convenience of relative imports, but that's just me.

We certainly could do this, but to enforce it we'd need another hook that runs before isort to convert relative imports to absolute i.e., absolufy-imports.

[bjlittle]

TBH I'm at the stage where I'm kinda done with this PR.

Perhaps any additional changes could be made in a separate PR, when there is consensus for what's wanted? As it's a wee bit unclear to me what I'm aiming for otherwise.

[dopplershift]

So overall I'm good with this. I personally find the empty line between "cartopy" imports and relative imports to be odd, so I'd personally opt for (2) from the options above. That would have the benefit of shrinking this diff a bit.

Otherwise if there's better consensus around the current PR, I'm happy to merge.

[bjlittle]

@dopplershift Thanks. Okay, thumbs up this comment to confirm you guys are happy with Option 2, and we'll roll with that 😀

[bjlittle]

@dopplershift @greglucas @QuLogic Okay guys, thanks for all your help on this. Very much appreciated 🍻

I've now pushed the final changes to go with Option 2, and I also fixed-up the commits to tidy things up.

[bjlittle]

BTW I've managed to secure funding to go to SciPy 2023 this year 🥳

Are any of you guys going? If so, I'd love to hangout and catch-up 😄

[dopplershift]

BTW I'm managed to secure funding to go to SciPy 2023 this year 🥳

Are any of you guys going? If so, I'd love to hangout and catch-up 😄

I'm currently planning on attending. Looking forward to seeing you there!