Fonttools: [Housekeeping] Formatting all code with black

[madig]

Hi all,
Cosimo and me would like to format all code with https://github.com/psf/black to unify style and automate the boring tasks of fitting your code into the surrounding style. This would also include a CI lint check to make sure the black style is adhered to for any incoming code.

Two known difficulties:

1. Existing PRs may need to be rebased on top of the newly formatted code.
2. Making git blame work as before requires an extra step: https://github.com/psf/black#migrating-your-code-style-without-ruining-git-blame

Thoughts?

[justvanrossum]

Making git blame work as before requires an extra step: https://github.com/psf/black#migrating-your-code-style-without-ruining-git-blame

There I read: "The one caveat is that GitHub and GitLab do not yet support ignoring revisions using their native UI of blame"

Which is literally the only way I ever use blame, so I'm not super excited at this moment.

[anthrotype]

even in github blame UI one can always click through past commits when exploring the git blame history, so I don't think this is such a problem. We have a lot of noisy commits where we do "minor" (whatever that means) or apply formatting changes

[justvanrossum]

Ok, fair enough. And as a side effect we'll lose all tabs, yes? That'd be worth a party.

[alerque]

My philosophy on this is that the sooner code styling gets normalized the sooner you get to the end of the long tail of disruption caused.

1. Existing PR's should be easy to fix one of two ways depending on complexity:
   a) If they can be automatically rebased on master now, they can be rebased automatically after the change by running the reformatter on each of the commits in the PR first, then rebasing on the new master.
   b) If the PR cannot currently be rebased anyway but it could be merged (for example git rerere artifacts have already been developed for it) then merge the old master pre-rewite into the PR branch to resolve conflicts, then run the formatter in the branch, merge again from the new master, then squash the two merges and format commit to get a clean merge from the new format.

2. Is web UI blame usage frequent enough that the extra click/level of blame history would really be a bother?

[behdad]

Sgtm.

[khaledhosny]

One thing I really hate about code formatters, is that they sometimes make more noise than reducing it. Contributor A sends a PR with lots of commits, CI fails because of code format, contributors A runs black after their last commit (because rebasing and editing every commit to reformat it is likely to cause merge conflicts and will require a lot of effort) and push the results, CI happy, reviewers happy and we now have an unnecessary noise commit that wouldn’t have been there.

[alerque]

(because rebasing and editing every commit to reformat it is likely to cause merge conflicts and will require a lot of effort)

Once the master branch is formatted automatically requiring it of PR's will not make merge conflicts more likely. It will require a little bit more effort, but it's also possible to put together a one-liner that fixes it automatically (by applying a filter to a rebase that works backwards through each commit applying the formatting until the branch point). That can even be made into a CI job that fixes PRs, although that tends to be finicky and intrusive. Sure most people don't have the knowledge to do that kind of rebase off the top of their head, but pointing them to a one-liner that fixes their PR for when they couldn't be bothered to follow the guidelines in the first place isn't too much to ask.

[khaledhosny]

Sure, but that is not what I see in some other projects, hence why I’m raising this point.

[madig]

Mh. Yes, that may happen, but I'm not particularly worried. As Cosimo pointed out, fT's commit log is pretty noisy already. It is also possible to run black in tox, so people remembering to do that may not hit the problem :)

[moyogo]

As far as I know, none of the projects I contribute to that require to run tox -e lint document it.