[Proposal] Black as python formatting library

[ppanero]

Motivation

As a developer I want to avoid making formatting changes that might modify the history based on my local configuration/preference on coding.

Therefore, the aim is to align all python modules with a common formatting/code styling. In order to achieve said goal, Black is the proposed formatter.

Summary

To use Black as Python formatter it would need to be added to the test suite in the run-tests.sh, and the corresponding changes would need to be applied to isort, pep8 checks.

In addition, guidelines on how to auto-format using black should be provided for the main IDEs (VS Code, PyCharm).

Resources

• Black documentation
• Git rewrite history

On-going RFC writing here: https://codimd.web.cern.ch/b9Ok5Jt2Q6OSRI31wNI6Tg

[fenekku]

So the RFC process says:

Fight for what you believe, but gracefully accept defeat.

... I wouldn't be following the spirit nor testing the process
if I didn't dissent :) .

Actually, I appreciate proposals like this one about tooling:
gives us the space to have some healthy discussions around it. I know this
is at the "request" stage and therefore it might be early or presumptuous
for me to give this feedback (I am not a CERN architect after all),
but I rather be honest from the get-go.

I also recognize I might be in the minority: at least 4 people seem pretty happy with this and
I am clearly the only public dissenter right now. I will try to be persuasive ;)

1- The premise is flawed

As a developer I want to avoid making formatting changes that might modify the history based on my local configuration/preference on coding.

The need for Black is couched as a way to prevent individual styling preferences from modifying code lines and therefore polluting the history of the changes.
And obviously the code lines mentioned here are ones that would not be related with the work at hand. I think we agree that modifying the style of lines
that are worked on is fair. These changes make sense in the history.

Well... one way to prevent that is to not style code you don't touch. Why would you style code you don't touch? ... because you are already using a code formatter aren't you?

I suspect this is the case because VSCode is being used. VSCode pesters us with autoformatters until we cave in and pick one.
But it turns out you can cancel autoformatting in VSCode:

"[python]": {
    "editor.formatOnPaste": false,
    "editor.formatOnSave": false
}

So, using Black is to solve a problem that afflicts some developers while imposing its effects on all developers and projects.

But perhaps Black has enough advantages to be a sensible tradeoff.

2- Black itself is problematic ... for us

Ok, so maybe VSCode highlighted how autoformatters are really neat and we want to use Black.

Why are autoformatters neat? Because they prevent tests/Travis from failing because of silly styling issues: inveniosoftware/flask-resources@b4aabf8 by fixing them automatically. Honestly, it is not enough of a big deal for me to force projects to use a tool to fix it. But again maybe it is for others.

So we would want the styling fixes to be localized (as established before) and automated.
Black automates it, but doesn't do localized changes: it changes lines that were not touched...
so it actually causes the problem.

Another formatter, AutoPep8 is actually much more subtle:
it only strives to correct PEP8 styling errors. That means it will only do localized changes, because as a developer you are the one who introduces PEP8 errors since the code you start with has already been linted.

And now for a grab bag:

• Maybe Black can be more subtle if configured properly, but
  • more config is more weight
  • seems like the 0-config approach was desired anyway
• Black forces changes on other tools
  • isort needs to be reconfigured
  • pep8 part of pytest too
  • contradicts line length of 80 of PEP8
  • even suggestion of git changes: add black to tests checks flask-resources#16 (review) (if we make the move) is more stuff to do...
• Black is in beta. That's not great. Requiring pre-release dependencies has brought its
  share of problem for us in the past.

[ntarocco]

... for me to give this feedback (I am not a CERN architect after all), ...
I also recognize I might be in the minority: at least 4 people seem pretty happy with this and
I am clearly the only public dissenter right now. I will try to be persuasive ;)

@fenekku I don't think you should feel like this, and I am sorry if this is the case! :) We (me for sure) really appreciate your feedback (not only yours but everyone's in particular from the people involved in the project) and we really value it! And my take is that the architects group will actually not decide anything, it will be this RFC process that will lead to a conclusion!
You are not the minority, I am personally also quite in doubt, evaluating pro and cons ;)

And now for a grab bag:

* Maybe Black can be more subtle if configured properly, but
  
  * more config is more weight
  * seems like the 0-config approach was desired anyway

* Black forces changes on other tools
  
  * isort needs to be reconfigured
  * pep8 part of pytest too
  * contradicts line length of 80 of PEP8
  * even suggestion of git changes: [inveniosoftware/flask-resources#16 (review)](https://github.com/inveniosoftware/flask-resources/pull/16#pullrequestreview-431274245) (if we make the move) is more stuff to do...

* Black is in beta. That's not great. Requiring pre-release dependencies has brought its
  share of problem for us in the past.

My opinion is that if we go for Black, it has to be 0-config: we use a tool as it is meant to be used. On the other side, I do agree that we will have to tune other tools, but we already have config for them if I am not mistaken, so I don't see it as a real issue.

For the line length: here they explain motivation. Honestly, I personally find the current line length too short and very annoying, but this is just me since PEP8 has been designed and agree by people much more intelligent than me. I don't find very relevant to be compliant with PEP8 for the line-length... I don't see any advantage/disadvantage on this.

Finally, about the beta: Gmail was beta for years :) sorry I had to say it... Jokes aside, it looks to me an active and well maintained project checking their GitHub repo. They explain why the beta here.

@ppanero thanks a lot for writing it! My feeling is that in the RFC we could add more details on the why, and it should answer the questions:

• why do we want this? (there is some content, but we could add more, see below)
• what are the PROs in adopting it? The CONs?
• focus on user/developer value: is it going to add value or to create issues?

My thoughts:

PROs

• developer will not have to remember how to fix code formatting (this is a big plus for me, I am so annoyed by having to remember what indentation to add when breaking code in new lines, I really don't want to do that)
• the code is formatted in the same way, we just forget about that
• we could even have a pre/post commit hook that will auto format code

CONs

• my biggest doubt is: is there a lot of added value? If I format code in a way and then the next developer changes it, do we really care? Is it only a developer attitude to be perfectionist, or is there more value than that?
• requires to configure other tools (not really an issue for me! What about others?)
• an entire code reformatting of each repo will introduce a "noise" commit in the git blame. Personally, I am not using much git blame, line blame, but I use much more file history, so it won't be much of a problem for me.

WDYT?

[fenekku]

Thanks @ntarocco for the summarizing PROs/CONs: that's pretty much where I stand! (with PROs weighting less for me personally).

[lnielsen]

I'm rebooting this one as I see style discussions coming up more and more often these days, and we're onboarding more and more developers as well that I see have troubles adapting to the style.

My personal opinion is that I find styling discussions some of the least useful discussion to have in a project. Like import sorting and other tedious tasks, your editor should do the job for you. Some of the previous issues with black have now also been resolved:

1. Black is stable and the have a process of only introducing new styling changes once a year.
2. Git blame can now exclude a specific revision: https://black.readthedocs.io/en/stable/guides/introducing_black_to_your_project.html#avoiding-ruining-git-blame

Therefore I would suggest we start introducing Black. I would go with a staged approach similar to moving from setup.py to setup.cfg. Meaning:

1. Implement changes in cookiecutter-invenio-module
2. Document with examples in an RFC how to perform the required changes.
3. Start moving repositories to black using the recipe when working on a repository.

This means we don't have to keep everything in sync, and the cost of adoption is spread over a long time. This do mean we will have some inconsistency between repositories, but I think that's acceptable as I don't see we'll have time to introduce it on all repos at once.

I would go with default black config without any changes.

Any thoughts?

[ntarocco]

I totally agree, we might have to check/change pycodestyle rules to be aligned.