Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

cesco-fran
Copy link

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

@augusto-herrmann
Copy link
Member

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

@benjello
Copy link
Member

benjello commented Mar 4, 2021

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

@cesco-fran
Copy link
Author

@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
Copy link
Member

bonjourmauko commented Mar 8, 2021

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

  • Its hideous.

Upsides

  • Automatically enforced, @benjello nobody would have to care anymore about coding in a specific convention, less overhead, more code, more productivity, you have plugins for VSCode that format on save;
  • It's easily enforceable on all "core" or "shared" OpenFisca repositories, so to facilitate homogeneity and thus contributors willingness to contribute, while other specific systems or packages can have their own style convention;
  • If you look sufficiently enough to something you don't like, you end up by liking it.

@cesco-fran
Copy link
Author

cesco-fran commented Mar 8, 2021

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
Copy link
Member

@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
Copy link
Author

cesco-fran commented Mar 9, 2021

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
Copy link
Member

benjello commented Mar 9, 2021

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
Copy link
Author

cesco-fran commented Mar 10, 2021

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Author

@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 bonjourmauko added policy:RFC Request For Comments: chime in! kind:improvement Refactoring and code cleanup labels Apr 1, 2021
@bonjourmauko bonjourmauko changed the title Paint it Black: Apply black style formatter to the python source code [RFC] Paint it Black: Apply black style formatter to the python source code Apr 1, 2021
@bonjourmauko
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:improvement Refactoring and code cleanup policy:RFC Request For Comments: chime in!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants