Eliminate default

[hadley]

No description provided.

[hadley]

I think the default method is dangerous and it would be better to throw an error. Yes, it's good to be permissive in what you expect, but accepting anything is going a bit far for not good reason. Better to throw a clear error.

[russellpierce]

I think there is probably a design decision to be made here, and I'd like to understand your input better to increase the chances that this package is useful for someone other than myself.

If I'm reading you correctly, you are implying that the function should only accept arguments of a type that has a reasonable chance of being converted into a sensible delay (e.g. numeric, POSIXct, Period, character, and difftime) but fail outright for something esoteric like a glm model? Where would that leave NULL and logical?

Part of the reason I went so permissive was due to the expectation that naptime might find use in code where the delay is one of the least important running concerns and it is more important that the show go on. In that context, it is more dangerous for a bug in the code generating the delay to stop the show than it is to just carry on with a warning. However, I can see that might be a use-case with limited generalizability.

Thanks again for your time and input.

[hadley]

Basically, I don't understand when (say) someone would type naptime(glm(rnorm(5) ~ rnorm(5))) deliberately. Therefore, if they typed it, they must've made a mistake, and it's better to fail clearly and loudly so that the user knows something goes wrong. i.e. it's good to be liberal in what you accept, but it's bad to silently propagate mistakes (a warning isn't silent, but it is easily missed).

[russellpierce]

I decided to follow both roads a bit.

Someone might (effectively) type naptime("not implemented yet") if trying to trigger naptime in response to a 429 from an API that responded with "not implemented yet" where their documentation claimed you'd get an integer number of seconds. I agree that is horrendously unlikely that a similar error results in something like glm(rnorm(5) ~ runif(5)).

I agree that handling "not implemented yet" and glm(rnorm(5) ~ runif(5)) is not something most people would expect from sleep function that takes flexible inputs. So, I've enabled throwing errors in those cases as the default behavior.

However, I still see some potential utility in not allowing the sleep function to throw an error under any circumstances. So, to support that initial use case, I've got an additional option or param that most folk can feel entirely free to ignore.

In cleaning up I stumbled onto an edge with how difftime objects were being handled and that will be resolved in the above PR as well. So, if you, or anybody else is experimenting with the package, it would be wise to upgrade. I'll push the update to CRAN closer to the start of December just in case anything else pops up in the interim.

[hadley]

It's your package, but I think it's a dangerous design principle. Most of the most dangerous functions in base R (e.g. sapply() and unlist()) follow the same principle of never throwing an error, but just trying to work no matter how bad the input.

In your API example, a change to the API will now silently propagate and it'll be easy to miss that something is wrong with the underlying data source.

[russellpierce]

I hear you, and (mostly) agree, that's why I overhauled things so that the above referenced cases will now throw errors. Thanks for the input, I'm pretty sure with the revised policy of throwing errors in response to ill-mannered inputs other people will find this package more useful than they may have under the initial policy of eating all errors.