python-trio / trio

Trio – a friendly Python library for async concurrency and I/O
https://trio.readthedocs.io
Other
6.1k stars 332 forks source link

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

Open njsmith opened 7 years ago

njsmith commented 7 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

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().