Core: Format code with Black

[balloob]

Black is the uncompromising Python code formatter. By using it, you agree to cede control over minutiae of hand-formatting. In return, Black gives you speed, determinism, and freedom from pycodestyle nagging about formatting. You will save time and mental energy for more important matters.

Introducing Black would mean that we can disable all style related linting and beginners and advanced users alike will be able to move faster. I have been using a similar tool (Prettier) for home-assistant-js-websocket and really love it.

We would set it up as a commit pre-hook and on CI we would just validate that all code is according to Black.

Black requires Python 3.6 so that means that our development now requires Python 3.6 as a minimum. I am fine with that.

What do people think?

CC @home-assistant/developers

[MartinHjelmare]

We should also add a pre commit hook for isort to not have to bother with that.

[rytilahti]

Just to clarify, this does not affect the runtime python requirement, i.e. python 3.5 will still be kept as the lowest target version?

[Danielhiversen]

I think that will make it easier for new contributors.
On the other hand, the strict code format for HA has made me a much better programmer.

I made a test with Black default settings: https://github.com/home-assistant/home-assistant/compare/black?expand=1
I like most of the changes

[cdce8p]

Just looking at some of the changes from @Danielhiversen, I'm not so sure we should use it.

It's right that it might improve some code and make contributing for new devs easier.

However at least in my opinion each and everyone's code has a personal style to it that helps that dev in improving their own code later one. I for one just took a look at how some of my code would change and am not sure it's really worth it.
Some examples:

• Each import would get it's own line (I know that this is the PEP8 compliant way, but we haven't enforced it in the past and I don't think we should in the future)
• Regarding quotes (' and "): It seems that all ' are changed to "?, although the recommended way I though was to use ' for single and " for multiple words and sentences.
• At least the default settings doesn't take our character limit per line into account, example
• Function calls over multiple lines are split unnaturally, example
• It sometimes introduces unnecessary/unwanted indentation, example

Although I see the appeal of using something like Black, at least in the default state I think it would make some of our lives harder than they need to be. To be clear, that doesn't mean that it can't be a solution for some (new) devs, especially the ones new to python. I just don't think we should do these changes to most of our current code base.

---

Edit

• Second example for unnatural function calls link

[andrey-git]

I think using python 3.6 for development is a severe limitation that overcomes the usefulness of the formatter.

[awarecan]

@cdce8p according black's readme:

-l, --line-length INTEGER   Where to wrap around.  [default: 88]
-S, --skip-string-normalization
                              Don't normalize string quotes or prefixes.

[dgomes]

I agree with andrey-git

Ultimately we will have developers writing and testing with python3.6 and not realising that python3.5 will break their contribution.

[rytilahti]

My general opinion is that we should to move to enforce formatting (be it with black or something similar. With adjustments where/if necessary). First of all this is one step towards code consistency, which is good in general and make it less likely to have issues with hound and other style-guide issues, freeing the developer/reviewer time. Some commentary on earlier comments follows.

@Danielhiversen I don't think having a check for code compliancy will hinder one to become a better developer, in fact, I think having feedback (without a manual feedback loop) will even make it easier to achieve.

From @cdce8p:

Each import would get it's own line (I know that this is the PEP8 compliant way, but we haven't enforced it in the past and I don't think we should in the future)

This will make the code more readable, and will make it easier to review diffs where a new entry is added, so I'm all for it.

Regarding quotes (' and "): It seems that all ' are changed to "?, although the recommended way I though was to use ' for single and " for multiple words and sentences.

I think that was a brain-dead idea to begin with. Does this bring anything useful to the table, except causing confusion and more burden to figure out whether the quotes should be updated when changing some variable?

At least the default settings doesn't take our character limit per line into account, example

This is configurable setting, although I would welcome a higher line length limit (even more than the black's default) for more room to better variable naming. The pep8 states explicitly that it's fine to adjust this:

For code maintained exclusively or primarily by a team that can reach agreement on this issue, it is okay to increase the nominal line length from 80 to 100 characters (effectively increasing the maximum length to 99 characters), provided that comments and docstrings are still wrapped at 72 characters.

Continuing to @cdce8p:

Function calls over multiple lines are split unnaturally, example

It sometimes introduces unnecessary/unwanted indentation, example

Although I don't really like the style of the first of these, the second one makes it really easier to read (and to spot that there's a dict hiding underneath). So IMO this is okay. It is generally a good rule to avoid changing irrelevant lines, which this enhances.

A remark to @andrey-git's and @dgomes' opinion on versioning:

Ultimately we will have developers writing and testing with python3.6 and not realising that python3.5 will break their contribution.

I think this depends on what will happen if an older release of python is used. Can we trust tox/travis to take care of this? I don't think there exists a survey of what python version is used by developers, but I'd assume that Debian is the only stable distribution currently not shipping 3.6+. And I think it'd be fair to assume that a developer running Debian is using at least testing in after all (which ships 3.6.5), and this can be alleviated with external packages or pyenv if needed. Even still, I would like to see that commits w/o enforcing a commit hook are still allowed and leave that to automatic checkers, for those cases where you have no ability to have black around.

[balloob]

@cdce8p I think that with pep8 and pylint we're already enforcing a lot of style. I actually like the two changes that you linked. Both make it more readable by putting each parameter on individual lines. I definitely have asked this from people in the past.

@rytilahti about line length, you could always run locally with line length 120 and then change back to HA for committing.

If we use Black, I would prefer to use with default settings, not tweak it.

I think I have to agree with @andrey-git and @dgomes that the lack of Python 3.5 development is painful. It means indeed that any Debian install won't work out of the box, and that's what Raspbian, the Pi distro is based off. I guess we could write a black-as-a-service bot that would run black and push that commit to the PR (or hope someone else does this).

[scop]

Any idea if it would be possible/feasible to get Black to work with 3.5.x? At least it uses f-strings, but that wouldn't be too hard to change.

[balloob]

Migrating a project to 3.5.3 seems like a lot of work. I also wouldn't be surprised if the maintainers picked this for reason.

[balloobbot]

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates.

Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment :+1:

[balloob]

Going to close this. Will bring it up again once 3.6 is the default !

(We've been using Prettier on the frontend repo btw and people love it.)