search

inveniosoftware / rfcs

RFCs for Invenio.
https://rfcs.readthedocs.io
4 stars 15 forks source link

[Proposal] Black as python formatting library #34

Open ppanero opened 4 years ago

ppanero commented 4 years ago

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

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

fenekku commented 4 years ago

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: https://github.com/inveniosoftware/flask-resources/pull/19/commits/b4aabf8c42bed207c534aee7bb9fba0154c6c1ca 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:

ntarocco commented 4 years ago

... 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:

My thoughts:

PROs

CONs

WDYT?

fenekku commented 4 years ago

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

lnielsen commented 2 years ago

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 commented 2 years ago

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