openfisca / openfisca-core

OpenFisca core engine. See other repositories for countries-specific code & data.
https://openfisca.org
GNU Affero General Public License v3.0
168 stars 75 forks source link

[RFC] Paint it Black: Apply black style formatter to the python source code #977

Closed cesco-fran closed 2 years ago

cesco-fran commented 3 years ago

This PR explore the possibility of the use of the automated formatter black to handle formatting.

augusto-herrmann commented 3 years ago

Black seems to be a nice tool to standardize formatting. I did not know it before.

benjello commented 3 years ago

Hi @cesco-fran, we had a discussion about black here (sorry it is in french). It was about a country package though.

cesco-fran commented 3 years ago

@benjello thanks you for the link ... formatting is often a matter of habit, more then the specific formatting choices I think is important to be coherent with it and document were the choice diverge from standard one ... so if after 2 year and half things did not change and team do not want pick an autoformater could still be of worth to have a look at the kind of formatting incoherence the project still have ... for documentation on divergence your link have some points which could worth passing on the documentations ... I thinking for example where you make your points on having space around =.

bonjourmauko commented 3 years ago

I agree, I started documenting conventions here #960, here openfisca/country-template#97 and here openfisca/extension-template#32.

I love our styling choices, like the space around = and I found the enforced style by Black to be hideous but it has the benefit of being easy to enforce –I haven't yet found a way to enforce = programatically.

The way I see it:

Downsides

Upsides

cesco-fran commented 3 years ago

even if is not thought to be a flexible autoformatter there are some parameter that could help to make it close to what someone expect to be a good formatting ... but again as you notice there will be things that need to be changed on formatting and if most people on the project does not feel the upsides do pay this cost of loosing their preferred formatting then of course black is an available solution.

bonjourmauko commented 3 years ago

@cesco-fran Applying all parameters, to make it as close as possible to what we have now, would you paste un example? Just a file for exemple. I think it could help the conversation.

cesco-fran commented 3 years ago

There are documented and undocumented preference ... the second one are more difficult to consider but could be discussed as inline comment on demonstrative PR like this one .... for the documented here the ones that would be problematic for black.

; E128/133: We prefer hang-closing visual indents
; E251:     We prefer `function(x = 1)` over `function(x=1)`
benjello commented 3 years ago

I think black can be very useful and I would definitively adopt it for core and give up about enforcement of E251. But I do find hang-closing visual indents very useful for coding.

cesco-fran commented 3 years ago

@benjello in case the project maintain this two conventions as I mentions above could be useful to points reasons for that, for E251 that mean just copy past what you write in the discussion you linked to this PR .... and by the way one point that could help mitigate the problem you have with E251 could be with the use of a different syntax colors on your editor that make better readability for function(x=1).

bonjourmauko commented 3 years ago

I've been playing with this a bit, and it works mostly, but some pieces of code become harder to read IMHO, take a look at this example https://github.com/openfisca/country-template/compare/use-black?expand=1#diff-fb9edbaf90a4affc8fd6a64c7ab59fa1d92791c9045a23698e72a3a8bcdd7e35R30 :

Before

        return (
            + household.sum(basic_income_i)  # Sum the household members basic incomes
            + household("housing_allowance", period)
            )

After (without manual fixing)

        return +household.sum(basic_income_i) + household(
            "housing_allowance", period
        )  # Sum the household members basic incomes

Full diff here https://github.com/openfisca/country-template/compare/use-black?expand=1

Of course with a bit of manual refactoring it'd work, like assigning to variables to be sure lines do less than 88 length, but wouldn't it be defeating the purpose of the proposal?

Food for thught.

bonjourmauko commented 3 years ago

That being said: what works or doesn't for OpenFisca Core may or not work for country packages and extensions.

Re-reviewing this PR I realise formula writing has a very precise sweet syntax when it comes to operations between vectors, which is mostly absent in OpenFisca Core.

cesco-fran commented 3 years ago

@maukoquiroga I think the fact that black move the comment is a bug (or more precisely an unexpected behavior ) of black... an could worth reporting it to their github issue page ... so as in all package one have to deal with this ... and if there are many of this I agree the package should not be used... the same apply for the refactor need ... in the sense that if the need of refactoring is rare that will be a price I think worth paying but if is common that is not more....

bonjourmauko commented 2 years ago

@cesco-fran closing in favour of #1047, but please do not hesitate to reopen if you think it is not the case.