-
-
Notifications
You must be signed in to change notification settings - Fork 335
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
timeout functions now raise ValueError on NaN inputs #2667
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2667 +/- ##
=======================================
Coverage 98.11% 98.11%
=======================================
Files 118 118
Lines 16534 16539 +5
Branches 3001 3003 +2
=======================================
+ Hits 16222 16227 +5
Misses 254 254
Partials 58 58
|
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. 1 genuine comment and 2 filler comments.
Args: | ||
seconds (float): The timeout. | ||
|
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.
Just a comment that I really don't like how this is so easy to forget :S
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.
This sounds like something that can be auto-generated / have the signature displayed in another manner in the doc, once the public interface is fully typed.
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.
Actually, I just remembered changes like this should probably get a newsfragment!
Hah! I've been working so much solely on internal stuff I completely forgot this was a thing, good catch! Feel free to merge if it looks good otherwise 🚀 |
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.
Small nitpick. breaking
does feel a bit far out but honestly that is probably the best category.
I'll merge (or you can) after blocking CI is fixed.
newsfragments/2493.breaking.rst
Outdated
@@ -0,0 +1 @@ | |||
Timeout functions now raise `ValueError` if passed `nan`. This includes `trio.sleep`, `trio.sleep_until`, `trio.move_on_at`, `trio.move_on_after`, `trio.fail_at` and `trio.fail_after`. |
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.
I'd rather "NaN" over "nan"
Fixes #2493
Consolidated all
ValueError
tests into a single test.