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

Adopt a code formatter #706

Closed
magopian opened this issue Aug 3, 2018 · 4 comments · Fixed by #727
Closed

Adopt a code formatter #706

magopian opened this issue Aug 3, 2018 · 4 comments · Fixed by #727
Assignees
Labels
kind:refactor Refactoring and code cleanup

Comments

@magopian
Copy link

magopian commented Aug 3, 2018

Following #702, the question was raised regarding the adoption of an automatic code formatter like black.

Some discussion already started in openfisca/openfisca-france#1060 (comment)

@fpagnoux
Copy link
Member

fpagnoux commented Aug 22, 2018

There are at least 3 potential candidates:

  • black
    • fascist highly opinionated: doesn't allow any configuration except for line length. I could possibly see it adopted for Core, but some choices are really rejected by economists, at least in France.
    • Really fast
  • yapf
    • Offers more configuration, ans is able to enforce spaces around =
    • Seems a little less well maintained than black.
    • Doesn't handle the u prefix in strings
    • Hanging indent for closing brackets doesn't seem to work right now.
  • autopep8
    • Does much less than the other ones
    • Can't enforce spaces around =
    • Share conf with flake8
    • Slow

@fpagnoux
Copy link
Member

fpagnoux commented Aug 22, 2018

Looking at the output of the 3 on an already flaked code:

  • The main impact of black and yapf seems to be to enforce a max line length and to handle that consistently. In some cases, the result definitely looks nicer. In others, it's arguable.
  • autopep8 would just allow to automate the fix of flake8 errors we usually deal with manually.

I must say I'm so far not fully sure to see the benefits of an advanced code formatter. I would be fine with trying if other contributors see benefits in it, or if we think that formatters are so standard that not having one makes our codebase look dirty. But if I come back to #702, the two things we wanted to automate were:

  • Making sure no u' are added to the code.
    • Only black can automatically fix that, so we'd have to adopt all its conventions
    • A grep -e \\bu\' -e \\bu\" **/*.py could detect them, and there may be a way to autofix in shell to. Agreed, it's much more artisanal than a dedicated formatter.
  • Automatically add future imports to the code
    • Neither black nor yapf can do that
    • In 4 months, we won't care as we'll stop supporting Python 2

@fpagnoux
Copy link
Member

fpagnoux commented Aug 22, 2018

So to summarize my position: no strong opinion, will not reject a PR that enforce any code formatting. So if this matters to you (@magopian or anyone) and that a formatter would make you contribute more, go ahead!

(I also realize that I spent way too much time thinking about all that, which by itself is an argument for outsourcing all decision to a code formatter 😉 )

@bonjourmauko
Copy link
Member

Take a look at #727 and let me know of you think that could be a first step towards a stricter solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:refactor Refactoring and code cleanup
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants