-
Notifications
You must be signed in to change notification settings - Fork 75
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
Conversation
Black seems to be a nice tool to standardize formatting. I did not know it before. |
Hi @cesco-fran, we had a discussion about black here (sorry it is in french). It was about a country package though. |
@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 |
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 The way I see it: Downsides
Upsides
|
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. |
@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. |
There are documented and undocumented preference ... the second one are more difficult to consider but could be discussed
|
I think black can be very useful and I would definitively adopt it for core and give up about enforcement of E251. |
@benjello in case the project maintain this two conventions as I mentions above could be useful to points reasons for that, for |
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. |
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. |
@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.... |
@cesco-fran closing in favour of #1047, but please do not hesitate to reopen if you think it is not the case. |
This PR explore the possibility of the use of the automated formatter
black
to handle formatting.