Switch to black #1540
Switch to black #1540
Conversation
There was a problem hiding this comment.
Thank you for doing this! I pointed out a few places where we could tweak things to make the new formatting look better. Please feel free to commit this with or without those changes, and without further review from me -- I'd like to avoid having it stay open for too long, since it's such an annoying change to merge with.
|
|
||
| Very occasionally, yapf will generate really ugly and unreadable |
There was a problem hiding this comment.
black supports this too with # fmt: off and # fmt: on comments; maybe worth mentioning?
trio/_core/tests/test_ki.py
Outdated
| platform.python_implementation() == "CPython" and sys.version_info < | ||
| (3, 6, 2) | ||
|
|
||
| buggy_wakeup_fd = platform.python_implementation() == "CPython" and sys.version_info < ( |
There was a problem hiding this comment.
ugh, this is an ugly one. Doesn't really matter though. It might do a better job if you put the clauses in the opposite order, and surround with parens?
trio/_socket.py
Outdated
| @@ -338,10 +341,10 @@ def _make_simple_sock_method_wrapper(methname, wait_fn, maybe_avail=False): | |||
| async def wrapper(self, *args, **kwargs): | |||
| return await self._nonblocking_helper(fn, args, kwargs, wait_fn) | |||
|
|
|||
| wrapper.__doc__ = ( | |||
| """Like :meth:`socket.socket.{}`, but async. | |||
| wrapper.__doc__ = """Like :meth:`socket.socket.{}`, but async. | |||
There was a problem hiding this comment.
Maybe make this into an f-string so it doesn't format so poorly?
trio/tests/test_util.py
Outdated
| @@ -53,11 +58,13 @@ async def wait_with_ul1(): | |||
|
|
|||
| def test_module_metadata_is_fixed_up(): | |||
| import trio | |||
|
|
|||
| assert trio.Cancelled.__module__ == "trio" | |||
| assert trio.open_nursery.__module__ == "trio" | |||
| assert trio.abc.Stream.__module__ == "trio.abc" | |||
| assert trio.lowlevel.wait_task_rescheduled.__module__ == "trio.lowlevel" | |||
| import trio.testing | |||
There was a problem hiding this comment.
With the extra blank lines, I think it looks better to put both imports next to each other.
|
Oh, and I believe you can fix the CI issues by only installing black on CPython. (It depends on typed-ast, which doesn't compile under PyPy.) |
Codecov Report
@@ Coverage Diff @@
## master #1540 +/- ##
=======================================
Coverage 99.67% 99.67%
=======================================
Files 108 108
Lines 13356 13358 +2
Branches 1012 1012
=======================================
+ Hits 13313 13315 +2
Misses 28 28
Partials 15 15
|
|
I'm neutral on whether we switch to 88 or stay at 80. (I also use an approx width=88 editor setup.) But I think if we plan to reflow we should do it now, to consolidate all the disruptive changes in one place. |
Agreed. I'm happy to wait for consensus: it's easy to rebase this as the actual black changes are isolated in their own commit. |
|
Oh, I wasn't bothered, just terse b/c I was replying from my phone while waiting for my laptop to upgrade to Ubuntu 20.04 :-). I'm in favor of switching to black, and I guess mildly in favor of sticking to the defaults instead of fighting them. |
Unfortunately, full black compliance is not possible, as a few lines are too long. This is why I added the `fmt` pragmas.
It depends on typed-ast, which isn't supported on PyPy.
trio/_core/_io_kqueue.py
Outdated
| @@ -84,8 +81,7 @@ def monitor_kevent(self, ident, filter): | |||
| key = (ident, filter) | |||
| if key in self._registered: | |||
| raise _core.BusyResourceError( | |||
| "attempt to register multiple listeners for same " | |||
| "ident/filter pair" | |||
| "attempt to register multiple listeners for same " "ident/filter pair" | |||
There was a problem hiding this comment.
combine these into a single string (here and below) -- you might want to grep for ".*" ".*" to see if there are any I missed
| buggy_wakeup_fd = ( | ||
| platform.python_implementation() == "CPython" and sys.version_info < | ||
| (3, 6, 2) | ||
| sys.version_info < (3, 6, 2) and platform.python_implementation() == "CPython" |
There was a problem hiding this comment.
This can go back to the original order now that we have longer lines
There was a problem hiding this comment.
Unfortunately, with the other order Black produces the original ugly code with each number on its own line.
trio/_socket.py
Outdated
| "Only available on platforms where :meth:`socket.socket.{}` " | ||
| "is available.".format(methname) | ||
| f"Only available on platforms where :meth:`socket.socket.{methname}`" | ||
| " is available." |
There was a problem hiding this comment.
Can this space move to the end of the previous line now that we have more room?
trio/_socket.py
Outdated
| @@ -497,8 +497,7 @@ async def _resolve_address(self, address, flags): | |||
| elif self._sock.family == _stdlib_socket.AF_INET6: | |||
| if not isinstance(address, tuple) or not 2 <= len(address) <= 4: | |||
| raise ValueError( | |||
| "address should be a (host, port, [flowinfo, [scopeid]]) " | |||
| "tuple" | |||
| "address should be a (host, port, [flowinfo, [scopeid]]) " "tuple" | |||
trio/_sync.py
Outdated
| @@ -321,8 +317,7 @@ def release_on_behalf_of(self, borrower): | |||
| """ | |||
| if borrower not in self._borrowers: | |||
| raise RuntimeError( | |||
| "this borrower isn't holding any of this CapacityLimiter's " | |||
| "tokens" | |||
| "this borrower isn't holding any of this CapacityLimiter's " "tokens" | |||
We now have 88 characters.
Trying again after #768 in 2018. Why now? black is much more popular, and Trio supports the same versions of Python as black. (It does not support PyPy, though.)
The benefits:
The downsides: