Products.cmfplone: PLIP: use an autoformatter to keep our code base (a bit more) consistent

[gforcada]

PLIP (Plone Improvement Proposal)

Responsible Persons

Proposer: Gil Forcada Codinachs (gforcada)

Seconder: up for grabbing

Abstract

• use black to format our code base
• revamp our code analysis tooling to not conflict with black formatting, i.e. flake8 and isort specially
• add the necessary CI infrastructure to ensure code keeps properly formatted, and warn otherwise

Motivation

tl;dr; efficiency, i.e. once and for all, end up all the formatting discussions and make contributing to Plone even easier.

Plone is transitioning into Python 3 and has decades of history on its shoulders. Linters and styleguides have helped keep some code consistency, but just like tendencies, they keep changing over time, and most of the times are left half implemented.

As everything, it starts small and gets more and more complex over time:

• first of all linters appeared creating reports of what should be fixed
• some time later, those linters (or new tools), were able to fix some of those errors reported by linters
• finally, complete formatters emerged, making consistency on huge code bases (like Plone) feasible

Autoformatters are already mature enough to deal with large code bases. This means, that for the first time ever, we are in a position where we can finally have a single source of truth on how to format our code base.

As usual though, that gets us only up to a certain degree, no functions, methods, classes etc. will get renamed.

Black is a python code autoformatter that has to main promises:

• ensures that the formatted code is logically the same as the unformatted code
• applies the same formatting rules everywhere

Notice that having the code formatted automatically (by black or whichever other tool) does not necessarily mean that we do not need any code analysis tool anymore. A bare exception handling is a really bad thing that one should never do, code analysis tools will warn about it, auto formatters will never.

Fortunately for us, it is not too much effort to make them cooperate with each other :+1:

Assumptions

• the framework team and the wider Plone community agrees on following the formatting style that black outputs
• we acknowledge that code formatting is best left to tools rather than on each and every single plone contributor
• code formatting and code analysis can, and must, co-exist, black is no replacement of flake8/pylint/etc but they need to be made cooperative rather than conflictive

Proposal & Implementation

• documentation on how to turn a package's code base into a properly blacked code base
• CI tools improvements (mr.roboto & jenkins) to ensure that black formatting is respected on new contributions
• update on our code analysis tooling to ensure they don't provide conflicting reports with black formatting

Deliverables

• documentation on how to run black on code packages
• CI improvements
• new code analysis defaults that do not conflict with the auto formatter

Risks

We keep the current half mess of styleguides and formattings, as well as linting hints on and off here and there.

We hinder contributions from newcomers by making them uneasy on how they are expected to format their contribution.

Participants

• Gil Forcada
• add yourself here

[ale-rt]

Well, my favourite tool for this is https://github.com/google/yapf, which is much more configurable and IMHO produces better code.
Anyway if we decide to go for black I am all in because I am 100% sure that adopting it will be a big benefit.

[jensens]

I second using black.

@ale-rt if yapf is your choice just configure it in a way its doing the same as black.

But my experience with yapf is its much more difficult to use and, most important, it allows too many options leading to discussions which this PLIP intention is to avoid.

[ale-rt]

If we decide to go black I will switch to black with no problem!
My personal evaluation lead to YAPF exactly because it is more configurable, but if the majority picks another tool I will switch to it without any problem.
I see much more an advantage to have this PLIP approved with black than having nothing like the current status is.
So if everybody loves black, let's go black :)

[davisagli]

+1 for black. The lack of configurability (and thus lack of bike-shed-ability) is a feature.

[tisto]

+1 for black. Though, we might want to wait with that until we get rid of Python 2. Since black does not run on Python 2 we always risk that people who work with Python 2 are not able to run the formatter.

[tisto]

@gforcada the @plone/framework-team approved your PLIP in its meeting today. Please go ahead with the implementation.

[jaroel]

+1 for black. Though, we might want to wait with that until we get rid of Python 2. Since black does not run on Python 2 we always risk that people who work with Python 2 are not able to run the formatter.

Black itself uses python3, but it will format both 2 and 3.

[mauritsvanrees]

In the issue linked by Jens just above, he says we can use black --check .
That could replace most of flake8. Not all of it: there are some plone/zope-specific flake8 plugins that we use.

[gforcada]

Yup, there are plenty of things that black does not check(i.e. a bare except: clause for example) that still needs flake8.

But there are quite a few flake8 plugins that can be disabled completely.

[jensens]

Sure, it does not check all.
Btw., now the PLIP is approved, what is next?

[gforcada]

Me finding some time to work on the deliverables? :sweat_smile:

[gforcada]

But, please, pretty please, forget about this PLIP for now, we have pretty much more specific and urgent topics to work on (i.e. get 5.2 out :wink: )

[idgserpro]

This will have implications with bobtemplates.plone, used to create packages in the Plone world. Should we try to enforce black there as well? For the sake of an example, if I run black in a newly created package from bobtemplates.plone, without --skip-string-normalization, it would create a big diff:

https://github.com/plone/bobtemplates.plone/blob/abb43795ed611138d451b61d05f0ec78b6e827d1/bobtemplates/plone/addon/setup.py.bob#L16

@MrTango FYI