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

Switch to black #768

Closed
wants to merge 4 commits into from
Closed

Conversation

pquentin
Copy link
Member

@pquentin pquentin commented Nov 5, 2018

We're currently using yapf to format our code, and this pull request is about switching to black instead.

The advantages of black:

  • the output is less likely to change when upgrading to a new version (this has been a problem for trio pull requests)
  • while # fmt: off / # fmt: on is supported, black indents nested dicts nicely, so I don't think we need that

The downsides:

  • contributors need to switch to the new tool
  • all pull requests will have conflicts :(
  • git blame will be less useful
  • it requires contributors to use Python 3.6+

The first commit prepares the switch, while the second one only runs black setup.py trio. Note that black uses 88 characters per line instead of 80.

What do you think?

@codecov
Copy link

codecov bot commented Nov 5, 2018

Codecov Report

Merging #768 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #768      +/-   ##
==========================================
+ Coverage   99.32%   99.33%   +<.01%     
==========================================
  Files          96       96              
  Lines       11636    11636              
  Branches      832      832              
==========================================
+ Hits        11558    11559       +1     
  Misses         57       57              
+ Partials       21       20       -1
Impacted Files Coverage Δ
trio/tests/module_with_deprecations.py 100% <ø> (ø) ⬆️
trio/testing/__init__.py 100% <ø> (ø) ⬆️
trio/_timeouts.py 100% <ø> (ø) ⬆️
trio/_core/_unbounded_queue.py 100% <ø> (ø) ⬆️
trio/tests/test_highlevel_serve_listeners.py 100% <ø> (ø) ⬆️
trio/hazmat.py 100% <ø> (ø) ⬆️
trio/_subprocess/linux_waitpid.py 100% <ø> (ø) ⬆️
trio/__init__.py 100% <ø> (ø) ⬆️
trio/_core/tests/test_io.py 100% <ø> (ø) ⬆️
trio/_core/_traps.py 100% <ø> (ø) ⬆️
... and 63 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ea6e44...9c8ad6a. Read the comment docs.

@Fuyukai
Copy link
Member

Fuyukai commented Nov 5, 2018

I support this soley on having longer line lengths.

@njsmith
Copy link
Member

njsmith commented Nov 5, 2018

I am nervous about longer line lengths, because I have all my editor and terminal windows set up to be 80 chars wide at the largest font that will let me put two of them side-by-side on my laptop screen :-). You can configure black to use shorter lines, though, and I can take a look at how hard it would be to move my configuration out of the stone age...

I wonder how much of an issue the 3.6+ requirement will be. We definitely have users who are running on Debian stable or similar where 3.5 is the easiest python to use. OTOH yapf still doesn't work consistently on 3.7 (google/yapf#624), so until that's fixed yapf is arguably worse than black in this regard. (At least black gives you a nice error if you run it on the wrong python version.) We can also potentially mitigate this with better tooling, e.g. a bot that runs black for you?

Last time I looked through the black output there were still some infelicities that jumped out at me. I was kind of hoping they'd get addressed before we switched:

https://github.com/ambv/black/issues/created_by/njsmith

But I guess @ambv has lots of things to worry about, and we're both distracted by this python governance mess...

Looking at the list of outstanding PRs, I think we want to get @jmfrank63's PRs merged first before doing anything here either way.

@pquentin
Copy link
Member Author

pquentin commented Nov 6, 2018

I don't have good answers to those questions. :)

I only know that if we do decide to switch to black, we do want to wait for @jmfrank63's PRs, yes, and any other active PRs: this PR is really easy to prepare again anyway.

I would be happy to close this and keep using yapf, I just wanted to make sure that the switch to black was not blocked because no one took the time to send a PR.

@ambv
Copy link

ambv commented Nov 7, 2018

I was kind of hoping they'd get addressed before we switched:

Well, 9 out of 11 issues you opened are already closed, it's not like I'm not trying ;-) I responded on both open issues now. In those cases it's not lack of time, but lack of clarity that is holding me back.

@pquentin
Copy link
Member Author

pquentin commented Nov 8, 2018

Thank you @ambv for your efforts!

@pquentin
Copy link
Member Author

Closing for now, it will be easy to reopen if needed.

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

Successfully merging this pull request may close these issues.

4 participants