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

Reformat all the code with black and enforce black coding style #1623

Closed
Kami opened this issue Nov 3, 2021 · 12 comments
Closed

Reformat all the code with black and enforce black coding style #1623

Kami opened this issue Nov 3, 2021 · 12 comments

Comments

@Kami
Copy link
Member

Kami commented Nov 3, 2021

I think it's time to finally move away from our "home grown" coding style and rules and move to something better a lot of other Python projects use (aka black - https://github.com/psf/black).

The actual commit / change which reformats all the existing code will be disruptive, but I don't think there is much we can do it and we should probably do it sooner than later.

Main problem with such change is that it will likely cause issues with some of the open PRs and in issues with forks which have non-upstreamed changes (aka are out of sync with upstream) - there may be merge conflicts which will need to be manually resolved when trying to get PR / fork in sync with upstream.

I think now it's about the right time since most of the active PRs will be merged soon so this will cause the least disruption in this regard.

We did a similar change in another project not too long ago (StackStorm/st2) and it wasn't too disruptive. But the difference is that StackStorm is a platform and not a library and there are likely not that many out of sync forks out there compared to Libcloud.

@Kami
Copy link
Member Author

Kami commented Nov 3, 2021

@apache/libcloud-committers @dpeschman @RunOrVeith @dimgal1 What do other people think?

I guess @d-mo you may have the most to say since I imagine you may use a fork which is not fully in sync with upstream.

@tonybaloney
Copy link
Contributor

👍🏻 to this change, but be careful with which version of black, because this can cause inconsistencies with people running locally,

@Kami
Copy link
Member Author

Kami commented Nov 3, 2021

@tonybaloney Thanks.

Do you happen to know what's the currently most used version of black (19.x?). I did notice recently when upgrading some otherprojects to the latest version that some of the formatting rules did indeed change which caused quite lot of code churn again.

But in any case, we plan to pin it in tox environment and advise developers to use tox command and which black version should be used.

@RunOrVeith
Copy link
Contributor

If we take care to minimize the amount of open pull requests I am open to it.
I personally prefer black with a longer line width than 79 characters (e.g. 120), because I think enforcing short lines encourages non-descriptive variable names, but I'm not too opinionated on it.

@micafer
Copy link
Contributor

micafer commented Nov 4, 2021

I fully agree with @RunOrVeith about setting a longer line width.
In many cases short lines make the code difficult to read.

@Kami
Copy link
Member Author

Kami commented Nov 4, 2021

I'm +1 for longer lines. I think 100 is a good middle of the ground compromise between 80 and 120.

@dimgal1
Copy link
Contributor

dimgal1 commented Nov 4, 2021

I'm +1 for black, 100 characters seems fine.

@dpeschman
Copy link
Contributor

I’m not that into it, but can see how it might benefit a project where many hands come and go.

@Kami Kami mentioned this issue Nov 4, 2021
3 tasks
@Kami
Copy link
Member Author

Kami commented Nov 24, 2021

Alright, #1624 has been merged and that includes more reasonable 100 character long line length.

Hopefully people won't have too many issues syncing their forks with the latest trunk version.

The best workflow probably is to run black with the same config as trunk uses on your local fork / changes, commit the change and then merge / pull in in latest upstream trunk - this should make addressing any conflicts easier.

So something along those lines:

# Run this inside the directory of your fork
wget https://raw.githubusercontent.com/apache/libcloud/trunk/pyproject.toml
# NOTE: It's important that the same version of black is used
pip install "black==21.10b0"
black --config pyproject.toml libcloud/
black --config pyproject.toml docs/examples/
black --config pyproject.toml demos/
black --config pyproject.toml contrib/
black --config pyproject.toml pylint_plugins/
git commit -am "Reformat code with black".
rm pyproject.yoml
git pull upstream trunk

I will add comment with similar instructions to all the open PRs when I get a chance to write it up.

@d-mo
Copy link
Contributor

d-mo commented Nov 26, 2021

Thank you @Kami for the heads up and for going ahead with this. It will inevitably introduce some merge conflicts but we'll manage. It's also a good opportunity for grooming and contributing back more changes from our fork.

@Kami
Copy link
Member Author

Kami commented Nov 26, 2021

@d-mo You are welcome and I hope it won't cause too much pain for you.

I tested the workflow above on an out of date fork and it "mostly works" - it looks like there is a bug in black with unstable code formatting (aka if you run black twice) in some edge cases which results in some more work / conflicts, but sadly there isn't much we can do (and black bug fix for that issue is not available yet).

@Kami
Copy link
Member Author

Kami commented Dec 5, 2021

Resolved via #1624.

@Kami Kami closed this as completed Dec 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants