Skip to content
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

Is it better to have sleep(0), sleep(x), sleep(inf), or checkpoint(), sleep(x), sleep_forever()? #310

Open
njsmith opened this issue Aug 26, 2017 · 4 comments

Comments

@njsmith
Copy link
Member

njsmith commented Aug 26, 2017

Speaking of cleaning up names and namespaces, sleep_forever is also accessible as sleep(inf) or sleep_until(inf). Does it really need a top-level name? And frankly the sleep_forever name is kinda confusing – wouldn't wait_cancelled or something make more sense?

Maybe we should make it so the inf versions of sleep special case this for the marginal efficiency gain (like sleep(0) does), and then make that the One Way of getting these semantics.

@smurfix
Copy link
Contributor

smurfix commented Oct 12, 2017

IMHO overlaying sleep with both "provide a cancellation point" i.e. sleep(0) and "wait until cancelled" = sleep(inf) is somewhat less than optimal because when I'm scanning the documentation for a function that provides one of these concepts I'm not searching for "sleep" – I'm likely to look for "suspension point" or "forever" respectively.

Thus I'm mildly in favor of keeping "special" functions for these purposes, if only as aliases.

@Zac-HD
Copy link
Member

Zac-HD commented May 26, 2018

Maybe we should make it so the inf versions of sleep special case this for the marginal efficiency gain (like sleep(0) does), and then make that the One Way of getting these semantics.

I'm strongly in favor of this change, primarily for pedagogical reasons - instead of presenting the 0 and inf cases as special, they can be treated just like any other call to sleep. sleep already has the "provide a cancellation point" and "wait until cancelled (or time is up)" semantics for any possible value, and providing a single function for this will make it more obvious to users.

Special-casing the implementation(s) is probably a good idea, so long as the code is reasonable. Magic clocks might also need to have distinct handling for infinite sleeps or timeouts?

@njsmith njsmith changed the title Get rid of sleep_forever Is it better to have sleep(0), sleep(x), sleep(inf), or checkpoint(), sleep(x), sleep_forever()? Sep 8, 2018
@njsmith
Copy link
Member Author

njsmith commented Sep 9, 2018

See also #655, where @joernheissler quotes the bits of the doc talking about sleep(0) as an idiom for a checkpoint, and writes:

I think it would be cleaner to add an explicit checkpoint function

I can see the arguments both ways here: sleep does have the checkpoint and sleep_forever functionality regardless, and parsimony says, make that the one-and-only-one way to perform the less common operations. OTOH, if we're feeling the need to specifically point out these options in the docs, maybe that's evidence that they're conceptually distinct and having different functions will be better for discoverability and clarity.

This is basically a tiny style judgment call, so I'm going to procrastinate on it for now. We've got the "potential API breaker" tag on it, so we'll definitely see it again before 1.0, if not before. And maybe with more experience we'll have a better "feel" for which makes sense.

@oremanj
Copy link
Member

oremanj commented Mar 18, 2019

I find myself writing await trio.hazmat.checkpoint() rather than await trio.sleep(0), so I'd support lifting checkpoint out of hazmat. Renaming sleep_forever to wait_cancelled makes sense to me. I think the teachability benefits of having different functions for different ways people are likely to think about what operation they want outweigh the minimalism/only-one-way-to-do-it benefits of special-casing particular arguments to sleep().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants