-
-
Notifications
You must be signed in to change notification settings - Fork 343
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 PosArgT typing to trio.run #3022
Conversation
f6e62ed
to
e668396
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3022 +/- ##
=======================================
Coverage 99.63% 99.63%
=======================================
Files 120 120
Lines 17865 17865
Branches 3216 3216
=======================================
Hits 17800 17800
Misses 46 46
Partials 19 19
|
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 to me!
Should we consider moving over some of the trio-typing test suite, since apparently that catches some issues that the trio tests don't?
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.
Must have been missed, yes.
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.
Maybe add a type test.
Feel free to merge this whether or not you add one though.
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 great!
There's not much that's worth adapting, I don't think we want to adapt their method of running internal mypy functions, so it would consist of copying and adapting the test-data. But we already cover most everything:
The only things left were |
I noticed this when going through the trio-typing test suite. I don't see any reason why this would've been intentionally skipped in #2881, so I suspect it just got missed?