kivy / python-for-android

Turn your Python application into an Android APK
https://python-for-android.readthedocs.io
MIT License
8.29k stars 1.84k forks source link

Discussion: introducing Black code formatter #1950

Open AndreMiras opened 5 years ago

AndreMiras commented 5 years ago

As the number of regular contributors increase, code styling starts to becoming an issue. Hence we've been wondering if we should introduce Black, The Uncompromising Code Formatter. Dusty Phillips properly summed up why I'd like us to introduce Black by saying:

Black is opinionated so you don't have to be.

I think this is why we should love and hate it at the same time. The main drawback I see from using it is it would completely mess up the git blame. This is not a big issue, but I often git blame to understand undocumented features, understand context or track how bugs get introduced.

ghost commented 5 years ago

To me it seems a bit unnecessary (since at least in my mind we're pretty consistent right now with the help of pep8 testing and pull request reviews) and what I'm a bit worried about is that so far all formatters I have encountered reformatted something unreadable/nonsensical in rare corner cases and I really dislike sinking time into such issues.

However if the others want it then I'm in, I can see the appeal and I wouldn't want to block the team's opinion in any way :scream: and you all know me I'm relatively style-uncaring anyway so I'm not the best person to weigh in on this :joy:

inclement commented 5 years ago

and what I'm a bit worried about is that so far all formatters I have encountered reformatted something unreadable/nonsensical in rare corner cases

As posted on #1918, a nice example of something black messes up is:

with mock.patch('sys.version_info') as fake_version_info, \
     mock.patch('pythonforandroid.entrypoints.handle_build_exception') as handle_build_exception:

which it wants to change to:

with mock.patch("sys.version_info") as fake_version_info, mock.patch(
        "pythonforandroid.entrypoints.handle_build_exception"
    ) as handle_build_exception:

(To be fair, this may be partly due to the unnecessarily long line involved.)

inclement commented 5 years ago

For my own opinion, I have nothing against black and I don't think it would be terrible to adopt it - probably the good and bad parts are at least fairly balanced. From the other direction, if I saw a project was using black it wouldn't put me off contributing.

However, on the whole I'm against it because I just don't think it's necessary, I really don't mind some style flexibility and I don't perceive it to be difficult to write reasonable code without black. This isn't a snap opinion, it's worth noting that we've discussed this before and the main reason I never made an issue like this is that I didn't want to advocate for black.

I would also consider the ugly structure of some of the core internals to be a much more major code style issue, one that can't (yet!) be resolved programmatically.

opacam commented 5 years ago

First of all, sorry I should have write here before...

I think that blackit's great to be used as an ortographic corrector, to solve some long lines, removing spaces, adding/removing new lines, unify single/double quotes...things like these and also to give coherency to the project...I tend to use it before commit but not always keep the suggested format by black...

So I'm afraid that I must agree with @jtc0de and @inclement, sometimes the format is odd, and a lazy format of all the code, could give us unexpected results. Maybe we could do a per-file format, reviewing all the code line by line and adding the proper exceptions in those lines that we don't want to be changed (black allow this)...but there is a lot of work in there...and I'm not sure if it's worth it

Also we have to consider:

So I would wait for now, maybe black it will be more mature in a few months or a better alternative arise and probably we will have less code to format...meanwhile...well...we just to stay tuned to the writing style whenever we review/write code, as always :joy:

@AndreMiras, thanks to bring this conversation in here :+1:

rdbisme commented 4 years ago

I use black very satisfactorily on all my projects (I also have it enabled by default on my vim) configuration. I think it makes the code way more readable and I would suggest to opt in with it. Despite being in beta status, it is very much ready to be used and also enables easier reviews since code style is automatically taken into account by it.

If you want I can try to make a PR to apply it (and also to fix linting checks to comply with it)

Julian-O commented 10 months ago

I am a huge advocate of Black. I have missed it dearly working on Kivy-related projects; the code just looks wrong when randomly formatted, and I miss being able to hit a button in my IDE and have it all re-formatted nicely.

To address some comments above: