lots of typing improvements #2682
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2682 +/- ##
=======================================
Coverage 98.84% 98.84%
=======================================
Files 113 113
Lines 16474 16507 +33
Branches 3004 3010 +6
=======================================
+ Hits 16283 16316 +33
Misses 134 134
Partials 57 57
|
trio/_sync.py
Outdated
| async def __aenter__(self): | ||
| await self.acquire() | ||
| async def __aenter__(self) -> None: | ||
| await self.acquire() # type: ignore[attr-defined] | ||
|
|
||
| @enable_ki_protection | ||
| async def __aexit__(self, *args): | ||
| self.release() | ||
| async def __aexit__(self, *args: object) -> None: | ||
| self.release() # type: ignore[attr-defined] |
There was a problem hiding this comment.
slightly illegal type: ignores, but would probably require some significant refactoring to get rid of.
|
whoa, nasty CI fails. Will look at that tomorrow or so |
There was a problem hiding this comment.
too many individual ones to comment on, but __exit__ and __aexit__ returns bool, not None. (or, well, it returns either True or anything else).
There was a problem hiding this comment.
too many individual ones to comment on, but exit and aexit returns bool, not None. (or, well, it returns either True or anything else).
FWIW, in typeshed these are sometimes typed as None also, just as precedent. These return types matter though: if one of these is typed as returning None or Literal[False], then mypy at least can infer some special stuff about the context managers as it knows they won't swallow errors. (refs: they reverted python/mypy#7577 but reachability is changed by this: python/mypy#7666)
This was just a quick review though!
| @@ -278,7 +280,7 @@ class SendStream(AsyncResource): | |||
| __slots__ = () | |||
|
|
|||
| @abstractmethod | |||
| async def send_all(self, data): | |||
| async def send_all(self, data: bytes | bytearray | memoryview) -> None: | |||
There was a problem hiding this comment.
Probably should be typing_extensions.Buffer, perhaps? Might be worth discussing in an issue on its own though, since it changes this core interface, widening it to any buffer-supporting class.
There was a problem hiding this comment.
You're very welcome to open it for discussion! I'm not aware of the nuances here at all, just copy-pasting types.
typing them to be returning |
|
ugh, ofc the docs build now requires docstrings for all Fixed the tests, I'd accidentally deleted a default value for a parameter |
Co-authored-by: Spencer Brown <spencerb21@live.com>
|
I think the issue there is that they need to have a definition in the documentation, so there's somewhere for them to link to. |
…see if that makes readthedocs happy
| @@ -492,4 +492,8 @@ def test_classes_are_final(): | |||
| continue | |||
| # ... insert other special cases here ... | |||
|
|
|||
| # don't care about the *Statistics classes | |||
There was a problem hiding this comment.
Instead of opting these out, wouldn't it be better to make them final? People shouldn't be inheriting from them.
There was a problem hiding this comment.
I don't really see any strong reason to forbid people from inheriting them either? I could plausibly see somebody writing a wrapper around one of the locks and them deeming it useful to inherit from the Statistics class.
| @@ -1096,6 +1096,8 @@ Broadcasting an event with :class:`Event` | |||
| .. autoclass:: Event | |||
| :members: | |||
|
|
|||
| .. autoclass:: EventStatistics | |||
There was a problem hiding this comment.
Instead of documenting the class itself here, it might be possible to just put an index directive in the statistics() method, so the name links back there. That way we don't have the same information repeated twice.
There was a problem hiding this comment.
After trying a bunch of different variants I can't get index to create a class-type reference. I can create an index named trio.EventStatistics that I can link to with :ref: - but I failed to create one I can link to with :class: which is what autodoc generates the docstring to do.
| async def __aenter__(self): | ||
| await self.acquire() | ||
| async def __aenter__(self) -> None: | ||
| await self.acquire() # type: ignore[attr-defined] |
There was a problem hiding this comment.
We can drop this attr-defined through fancy self-type schenanigans:
import typing
class ACM(typing.Protocol):
async def acquire(self) -> None:
pass
class AsyncContextManagerMixinWithError:
async def __aenter__(self) -> None:
await self.acquire()
class AsyncContextManagerMixinWithoutError:
async def __aenter__(self: ACM) -> None:
await self.acquire()Feel free to check (I find https://mypy-play.net nice for testing small things like this) :D
There was a problem hiding this comment.
Ooooh, fancy! Leaving it for a future PR, adding a comment with a #TODO, as I'm unused to Protocols.
jakkdl
dismissed
Fuyukai’s stale review
No followup review after changes have been adressed
|
I have started on a follow-up typing PR, so I'll merge this one tomorrow unless you have any final objections @A5rocks (or you can go ahead and merge it) and any more changes can be addressed in it. |
Lots and lots of trivial typehints, but a couple tricky ones hidden inside the huge diff >.>
I'll point some of them out in a review of my own.