A big downside is that this makes merging open PRs a lot harder.

Additionally we need to set up pre-commit or something like it to ensure things keep the expected formatting.

On the one hand I'd be highly in favor of reformatting, but I think we need a broad concensus among the regular committers.

we have a tox -e lint task that is run by the CI on PRs which already runs black --check --diff to check that (new) things are properly formatted, so we could do the same with isort I guess? Although this probably would only apply to the newly added/modified files. This PR on the other hand is applying these tools to the entire codebase, some very old files which haven't been touched in ages. By the way, didn't we run black on all files before setting up the CI job? I think we did, but of course black itself has changed..

There's the issue of open PRs becoming unmergeable that Just mentioned, but it could be worked around by running the same automatic tools on those PR branches to fix the merge conflicts I presume.

Running black should be safe, but for isort we need to make sure that the reodering doesn't mess up the functionality for module imports that have side effects, e.g. importing fontTools.subset adds the subset methods to table classes, or importing otTables defines the otData classes etc.

I think you also need to tell isort to follow black's profile otherwise they'll step over each other:

https://pycqa.github.io/isort/docs/configuration/black_compatibility.html

@madig added these to ufoLib2's pyproject.toml, have a look

https://github.com/fonttools/ufoLib2/blob/5db1b88ca7894cc41215af664a00be39ab90b08c/pyproject.toml#L53-L57

By the way, didn't we run black on all files before setting up the CI job? I think we did, but of course black itself has changed..

Yes, we did. I ran black after isort again only to clean up some things that isort formats differently, which black would correct then again, e.g. isort’s default style for wrapping long import lines is

from fontTools.cffLib import (TopDictIndex, buildDefaults, buildOrder,
                              privateDictOperators, topDictOperators)

while black does

from fontTools.cffLib import (
    TopDictIndex,
    buildDefaults,
    buildOrder,
    privateDictOperators,
    topDictOperators,
)

I see now that I could have avoided that by using the isort black profile. So this is really only about adding isort.

Although this probably would only apply to the newly added/modified files.

turns out I was wrong, I thought black's --diff flag which we use in tox -e lint would only run black on the diff (i.e. added/modified) but it actually means "show the unified diff in the console output".

If we go with isort, we should add something like isort --profile black --skip-gitignore --check-only --diff . to the [testenv:lint] section of tox.ini so that it gets run on PRs, similar to the exiting black --check --diff .

If you're going to change tools around, I highly recommend replacing black and isort with just ruff. It is just better and faster.

I also see the existing PR problem pointed out by Just, but unless someone goes through the trouble of reviewing and merging all existing PRs, I don't see how to make it go away. The only alternative I can think of would be to not autoformat. I'd see it as a change for all future PRs.

Oh, also, don't forget to add the formatting commit to https://github.com/fonttools/fonttools/blob/main/.git-blame-ignore-revs to keep the Blame view working well.

I highly recommend replacing black and isort with just ruff. It is just better and faster.

+1 to just switch to ruff (I just realized that's what my vscode is also using to autoformat on save)

technically ruff is not 100% identical to black (here's the list of known deviation https://docs.astral.sh/ruff/formatter/black/) but it's close enough for me not to even realise I was using that instead of black

The only alternative I can think of would be to not autoformat. I'd see it as a change for all future PRs.

we are already autoformatting with black, as Jens said, this PR only adds isort (PR title should be renamed for clarity). He ran black after isort because he had not set up isort to output black-compatible output.

Running isort only on future PRs for newly added or modified files is a bit impractical. The GH action would have to first identify all the newly added or changed files (e.g. using https://github.com/tj-actions/changed-files) and then run isort on those only.

Here's the ruff version of this PR: main...format-everything-mk2.