Add isort (import sorting) to pre-commit hooks

[dstansby]

PR Summary

This adds isort to the pre-commit hooks, to sort import sections. I've used this on several other projects, and think it's a really nice way to make sure import blocks are clear and consistent.

To start with I've applied this to tutorials and examples.

Thoughts?

PR Checklist

Documentation and Tests

• Has pytest style unit tests (and pytest passes)
• Documentation is sphinx and numpydoc compliant (the docs should build without error).
• New plotting related features are documented with examples.

Release Notes

• New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
• API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
• Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

[tacaswell]

I'm 👍 on doing it in the examples, +0 on applying it to the library (minor improvement in consistency, but one more thing that we hassle contributors about (a robot telling your code is formatted badly is better than a person telling you, but it it still something complaining about your code) and I have like a 0.01% concern that we have something in the library which is import order dependent because that is our luck 🤣 ).

[dstansby]

one more thing that we hassle contributors about (a robot telling your code is formatted badly is better than a person telling you, but it it still something complaining about your code)

The upside is that isort does make the fixes for you, instead of complaining and leaving you to sort it out yourself. I definitely hear this though - I've just applied to the tutorials and examples in this PR.

[rcomer]

Having been the human who asked someone to move an import the other day, I would have quite liked a bot to do it instead! I guess a human can explain why though, which is useful if the contributor is learning…

[greglucas]

It seems like we've been having a few of these auto-formatting conversations again lately, so perhaps we should open a more general issue to discuss? 👍 I am definitely in favor of these simpler things and feel it is a waste of human's time commenting on people's PRs just to tell them to lint this way or that.

I've been hearing a lot about Ruff lately: https://github.com/charliermarsh/ruff which seems intriguing to replace most of these smaller formatting packages, although I haven't used it myself yet.

[jklymak]

I don't feel strongly about this, but I'm not a fan of doing this in examples, at least the way it's been done here. I always do

import numpy as np
import matplotlib.pyplot as plt

First, isort adds a space.

import numpy as np

import matplotlib.pyplot as plt

which is a bit silly for two imports.

Second if there are more Matplotlib imports, it insists on ordering them.

import numpy as np

import matplotlib.artist as martist
import matplotlib.pyplot as plt
import matplotlib.scale as mscale

like there is no difference between them, where plt is definitely different, at least in my mind. I think

import numpy as np
import matplotlib.pyplot as plt

import matplotlib.artist as martist
import matplotlib.scale as mscale

is more readable: boilerplate followed by stuff that is different. Given that a whole bunch of these changes are for import orders similar to above, I think others over the years agree. Maybe there is a way to make exceptions?

For long import lists in the main library, I think this sort of thing is fine and our basic style.

[dstansby]

It's all fairly configurable, so thanks for the feedback! I agree with your comment that pyplot and numpy imports should go together, thoughts on the sorting now?

[ksunden]

@greglucas I played with Ruff on our codebase a little today (having a podcast episode in my queue about it), It is quite fast (~100 ms compared to ~8 seconds for flake8 with pretty similar configs, and flake8 pins all cpu threads while it works)

There seemed to be some of the checks from our .flake8 config that are not handled by ruff.
Several of these checks were flagged as not supported when they were already on our ignore list anyway, but not all.
Most puzzling to me was E201 (whitespace after (), which feels like a relatively easy to check (and fix)

Ruff is used by at least Pandas, as a pre-commit hook (though they have a rather large number of pre-commit hooks...)
I spot checked a few other projects on their "users" list, Cibuildwheel has an open PR for it, but seems to not be actively using already, despite being listed in Ruff's readme.

[oscargus]

Minor thing, but maybe the config can go in pyproject.toml instead?

[dstansby]

Minor thing, but maybe the config can go in pyproject.toml instead?

I thought about this, and thought it was more consistent (at the moment) with other config (flake8, pytest-cov) to have an individual file. On the other hand this is getting a bit bloated, so I could start the move towards putting stuff in pyproject.toml here? I'm +/-0 here, so opinions or decisions welcome.

pre-commit is passing locally for me, but CI doesn't like it... maybe CI is not picking up the config file? Does anyone have any ideas?

[rcomer]

Does this explain the CI failure? PyCQA/isort#2077

[I have not understood it - just seen it referenced in another repo.]

[ksunden]

I do not think that issue explains the CI failure here -- This CI was run before that release and that error would have presented as failure to install rather than changing all the files.

It looks to me like it doesn't get the config options when run on pre-commit ci, but that guess is purely based on the number of files touched, haven't gone and examined the results or anything.

[dstansby]

@pre-commit.ci autofix

[dstansby]

pre-commit.ci autofix

[dstansby]

It looks like pre-commit.ci isn't respecting the section ordering, and is trying to put matplotlib imports before numpy and matplotlib.pyplot imports. Locally the same config is trying to (correctly) do the opposite.

[dstansby]

🎉 explicitly listing the first party libraries as matplotlib in the config file worked.

[greglucas]

Do we want to put this setup as a tool in pyproject.toml instead?

[dstansby]

Since you're the second person to ask, I've moved it over to pyproject.toml 😄

[greglucas]

In a few files we import from both matplotlib and mpl_toolkits and because they are sections here they are separated with a space. Is that what we want, or do we want to consider mpl_toolkits a part of matplotlib and therefore on the same import section level? I'd vote for equivalence because we are in the same package, but I don't have a strong preference at all. I'd also probably suggest matplotlib before mpl_toolkits in the ordering (alphabetical and primary namespace).

Here is what it looks like currently, which may be a bit too spread out?

import matplotlib.pyplot as plt

import mpl_toolkits.axes_grid1

import matplotlib

[oscargus]

Agree on matplotlib before mpl_toolkits. Guess that there are arguments for both separating row or not, no strong opinion there, but I would not object skipping the empty row if possible (so a soft preference).

[dstansby]

matplotlib.pyplot lives next to numpy - I've changed this a bit to add a pydata section in the config, to make the intent clear. If we were using e.g. pandas or scipy they would live there too under general pydata packages that people are likely to import together at the top.

Good shout on toolkits, I've moved toolkits after matplotlib, and removed the line break beetween them.

[oscargus]

This seems to have lead to that the pyplot-numpy section is sorted in alphabetical order, so pyplot now comes before numpy. Although the pydata makes sense, is that the wanted outcome?

Maybe it doesn't make a real difference, just that it earlier was the other way around...

[greglucas]

👍 I like this organization and it is nice to establish consistency here.

[oscargus]

I think this looks fine. Just a minor question about the numpy/pyplot order. If the current result is the expected, OK for @dstansby to self-merge (or @greglucas to merge). If not, feel free to self-merge on green after changing it.

[dstansby]

I think the order is good 👍