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

Discussion: introducing Black code formatter #1950

Open
AndreMiras opened this issue Aug 1, 2019 · 6 comments
Open

Discussion: introducing Black code formatter #1950

AndreMiras opened this issue Aug 1, 2019 · 6 comments
Labels
feature-request need-discussion The issue or pull request needs a discussion to take a team decission

Comments

@AndreMiras
Copy link
Member

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.

@AndreMiras AndreMiras added the need-discussion The issue or pull request needs a discussion to take a team decission label Aug 1, 2019
@ghost
Copy link

ghost commented Aug 1, 2019

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 😱 and you all know me I'm relatively style-uncaring anyway so I'm not the best person to weigh in on this 😂

@inclement
Copy link
Member

inclement commented Aug 4, 2019

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

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

opacam commented Sep 15, 2019

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:

  • black is in Beta status, so things could change (for the better...or not...who knows)
  • We probably have an upcoming removal...Python2

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 😂

@AndreMiras, thanks to bring this conversation in here 👍

@rdbisme
Copy link

rdbisme commented Apr 10, 2020

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

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:

  • No longer in beta.
  • There are techniques to deal with Git Blame - e.g. you convert the project in one big bang, and then can tell GitHub not to include that commit in the blame history. Seems like black magic, but apparently works well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request need-discussion The issue or pull request needs a discussion to take a team decission
Projects
None yet
Development

No branches or pull requests

5 participants