Closed Bug 1594793 Opened 5 months ago Closed 2 months ago

Use Black to format Python code

Categories

(Webtools :: Pontoon, enhancement, P5)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: adrian, Assigned: adrian)

References

Details

Attachments

(3 files)

Black is a tool that automatically formats Python code to make styling coherent across a project — and actually across all projects that use Black. It is a superset of PEP8 rules. It is very opinionated (only a few things are configurable). Its goal is to improve code readability (especially diff readability) while removing the burden of enforcing code style from developers.

Documentation: https://black.readthedocs.io/en/stable/index.html

This bug covers:

  1. deciding if we want to use Black or not;
  2. agreeing on its configuration;
  3. formatting all our code with Black;
  4. enforcing that new code has been "blacked" during CI;
  5. documenting usage of Black for developers.

Here's an example of running Black on the pontoon folder:
https://github.com/mozilla/pontoon/compare/master...adngdb:reformat-python-code-with-black

I'm in favor of doing this. I'd like to get opinions from Matjaz, Pike and jotes. I'm also going to ask around if other Python devs at Mozilla have experience with it.

Flags: needinfo?(poke)
Flags: needinfo?(m)
Flags: needinfo?(l10n)

Early observations:

  • I like the output in the example you gave.
  • I don't like the size of the changeset, because it makes things like blame useless from the point in time we "black" back.
Flags: needinfo?(m)

I decided against prettier and black, for anything that I maintain. The pros and cons depend on personality type. I personally don't have a problem with making decisions around code formatting, I don't think that blacked code is easier to read either. OTH, I find it revolting if machines do stuff that I can't fix, or do stuff behind my back. Machines do what I tell them to, and not the other way around.

That's why these tools are a burden to me.

I also understand that it's the other way around for other people.

Flags: needinfo?(l10n)

I asked other Python devs at Mozilla, and the answer was a big yes. Notably, willkg blacked all of Socorro — https://github.com/mozilla-services/socorro/commit/842429ea6ce8bdda50535d4dfdb935f8af20a355.

Matjaz's concern is very valid. Will was also concerned about the size of that commit he landed. I think we can do it by module, doing one at a time and always ensuring that we don't create conflicts with open PRs. The "blame" problem we cannot work around, though.

Axel, I understand your concerns and I think there are risks indeed that on some cases, Black might be a pain, but I believe the value it brings overweights that, especially for a project that has several contributors and had contributors with different coding styles in the past.

FWIW, we do these changes on gecko on a regular basis, and there's a few lessons to learn from that:

One, rip of the band-aid. Just do it. Use .git-blame-ignore-revs to advocate revisions to ignore for blame. That file name is pretty common, so I suspect that at one point gh will also use it.

For rebasing, we used https://pypi.org/project/hg-formatsource/, but that's mercurial only. I haven't dug in deep to find a git version of it. AFAICT, it rebases towards the last good revision, then reformats the patch, and then rebases further. Or so.

Other question, if you're going with black, should you also buy prettier for the js side of it?

TLDR; :+1: for Black and other code formatters.

My 2 cents:
All Python teams at my company use Black (or going to use it) to format the source code of their projects. I'm a huge fan of code formatters/linters.
It makes my team's life easier because nobody has to remember all the formatting rules. All documents like e.g. code-style guidelines like to get outdated after some time because they require human dedication to follow them.
Pontoon's contributors base will grow in the near future. That's why it will be faster to review changes if all of them will be formatted in the same way.

Black is not compliant (in a few places) with the coding standards preferred in Pontoon and also I don't know a tool that would implement similar formatting rules (e.g. vertically aligned function arguments). Nevertheless, Black/any other code formatter has my seal approval.

Flags: needinfo?(poke)
Status: NEW → ASSIGNED

I'm realizing now, while I'm working on running Black checks during our CI process, that it requires Python 3.6+ to run, and thus we cannot install it in travis so long as we run Python 2.7. I'll need to postpone enforcing black formatting until we've moved to Py3.

Depends on: 1585253
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.