Conversation
Codecov Report
@@ Coverage Diff @@
## master #1873 +/- ##
==========================================
- Coverage 99.59% 99.21% -0.38%
==========================================
Files 114 115 +1
Lines 14574 14914 +340
Branches 1110 1115 +5
==========================================
+ Hits 14515 14797 +282
- Misses 42 96 +54
- Partials 17 21 +4
|
|
|
||
| @enable_ki_protection | ||
| def acquire_on_behalf_of_nowait(self, borrower): | ||
| def acquire_on_behalf_of_nowait(self, borrower: Union[object, Task]) -> None: |
There was a problem hiding this comment.
Union[object, Task] simplifies to object
There was a problem hiding this comment.
@altendky you probably want CapacityLimiter to be generic so you can do:
| def acquire_on_behalf_of_nowait(self, borrower: Union[object, Task]) -> None: | |
| def acquire_on_behalf_of_nowait(self, borrower: T) -> None: |
so you can do:
class MyResource:
def __init__(self):
self._limit: trio.CapacityLimiter[MyResource] = trio.CapacityLimiter(40)
def acquire(self):
self._limit.acquire_on_behalf_of_nowait(self)
def release(self):
self._limit.release_on_behalf_of(self)There was a problem hiding this comment.
the methods that use a task would have to refine the self type:
@enable_ki_protection
def acquire_nowait(self: _CapacityLimiter[abc.Task[object]]) -> None:
"""Borrow a token from the sack, without blocking.
Raises:
WouldBlock: if no tokens are available.
RuntimeError: if the current task already holds one of this sack's
tokens.
"""
self.acquire_on_behalf_of_nowait(trio.lowlevel.current_task())There was a problem hiding this comment.
Hints like Union[object, Task] were pretty intentional, but open to change. In this case it mirrored the documentation describing a Task as a common particular type of object used here. In other cases, code actually treated the specific type differently. Given the suggestion of a more complicated change that I'm not going to get to right now, I'll leave it as is for now.
With the scale of this PR I expect tasks like this to get lost. I'll make an explicit list of them in the OP with links. Maybe I'll get lucky and find it is pointlessly redundant.
If there's any interest in bulk contributing to the PR... I'd welcome it. :] There's lots more hinting to add, even as just a 'first pass' to satisfy not-so-loose Mypy settings.
| return self | ||
|
|
||
| async def __aexit__(self, *args): | ||
| async def __aexit__(self, *args: object) -> None: |
There was a problem hiding this comment.
shouldn't that be
async def __aexit__(self, exc_type: Optional[Type[BaseException]],
exc_value: Optional[BaseException],
exc_trackback: Optional[TracebackType]) -> Optional[bool]:As that is the type the contextmanager protocol uses
(Maybe with added = Nones, not sure about that)
There was a problem hiding this comment.
I don't think this helps anything, but it does seem more proper. Though I think -> None is an accurate and good expression of the behavior. Thanks.
Added to the list in the OP.
There was a problem hiding this comment.
using -> None would disallow subclasses from returning True if they have some reason for that
(returning None and False have the same effect)
There was a problem hiding this comment.
Good point. Thanks for following up.
|
Closing this as stale; it looks like we're taking a more incremental approach - so far in #2477, #2671, #2682, and generally with https://github.com/python-trio/trio/pulls?q=is%3Apr+label%3Atyping. I really appreciate all the work that went into this though! |
4 participants
Draft for:
nitpick_ignoresstubtestto help with verifying dynamic stuffCapacityLimitergeneric Merge type stubs #1873 (comment).__aexit__()hints Merge type stubs #1873 (review)