-
-
Notifications
You must be signed in to change notification settings - Fork 341
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
Add types to most of trio.testing
#2747
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2747 +/- ##
==========================================
- Coverage 98.93% 98.92% -0.02%
==========================================
Files 113 113
Lines 16752 16770 +18
Branches 3027 3027
==========================================
+ Hits 16574 16590 +16
Misses 123 123
- Partials 55 57 +2
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly just a bunch of nits, looks great overall! And imo the types increase readability of a lot of this code
trio/_tests/verify_types.json
Outdated
"trio.testing.memory_stream_pair", | ||
"trio.testing.memory_stream_pump", | ||
"trio.testing.open_stream_to_socket_listener", | ||
"trio.testing.trio_test", | ||
"trio.testing.wait_all_tasks_blocked", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If anybody else is confused: wait_all_tasks_blocked
is defined in _core/[_generated]_run.py
but exported in testing/__init__.py
and is removed from this file in #2733
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good except noted, most of noted is that I think a lot of places might accept memoryview
objects as well
trio/testing/_check_streams.py
Outdated
async def do_send_all(data): | ||
with assert_checkpoints(): | ||
assert await s.send_all(data) is None | ||
async def do_send_all(data: bytes) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If s
is a Stream object here, it actually accepts bytes | bytearray | memoryview
apparently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might, but it doesn't really matter - these are internal nested functions, they're only being called with bytes
objects.
Merged in master, and while doing so found out that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, though I'm not really able to review this too well without spending way more time than I'd like :(
(as a sidenote we should probably go through a couple of the tests and type them to make sure the user-side types are sane...)
@@ -2152,7 +2152,7 @@ def setup_runner( | |||
|
|||
|
|||
def run( | |||
async_fn: Callable[..., RetT], | |||
async_fn: Callable[..., Awaitable[RetT]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Geh. I assume there's no other things like this?
yeah, once we're done with the public API we should
I think there's diminishing returns in reviewing PRs extremely thoroughly and it's more or less guaranteed that some problems will slip through, but it's more efficient to just catch those later imho - esp as type errors don't affect functionality. |
# Conflicts: # docs/source/conf.py # trio/_tests/verify_types.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the ignores from pyproject.toml
:
# 2747
"trio/testing/_network",
"trio/testing/_trio_test",
"trio/testing/_checkpoints",
"trio/testing/_check_streams",
"trio/testing/_memory_streams",
remove testing/* files from the ignorelist in pyproject.toml for mypy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or I can just do it myself when there's no other problem :p
This adds types to
trio.testing
, except for_fake_net
(which is considerably more complex and should probably go in its own PR). It's fairly straightforward, though in a few cases I made functions forward on both*args
/**kwargs
so they can useParamSpec
, instead of just one or the other. For_check_streams
, to correctly type things the functions now expect/return tuples instead of other iterables that just happen to be 2-long - in practice I don't think people would do anything else anyway. It'll just fail type checking if so.