jeroen / openssl

OpenSSL bindings for R
Other
63 stars 20 forks source link

Could you move away from using stopifnot? #49

Closed m-stanley closed 6 years ago

m-stanley commented 6 years ago

Hello,

I'd like to register an impassioned plea for you to move away from using stopifnot in package functions - especially non-exported functions such as read_input, which is the cause of my use case today.

The reason for this is that stopifnot sets call = FALSE on its call to stop with no way to override that default, and this is extremely annoying when the function in question is used at the bottom of a very long call chain. and most especially when the function call involves cross-package usage.

In my use case, I was using the package secret, which uses openssl::read_key to read in keys. But because the path I was supplying to read_key was a directory and not a key (I had misunderstood the documentation for secret::local_key) the call to read_key of course failed.

secret::local_key() Error: !info$isdir is not TRUE

Because there is no stack trace thanks to call = FALSE, it wasn't immediately obvious what execution path had led to this situation or where the stopifnot that led to this message was being generated. This led to a several-minute spelunking expedition into the two packages and eventually into the non-exported function openssl::read_input to understand what the cause of the error was.

So, I implore you - please move away from triggering errors with no call attached in general, and move away from using stopifnot in particular, because it will make life much easier on your users who encounter those error messages.

jeroen commented 6 years ago

We need to raise an error when type checking fails. stopifnot is convenient, we could use some custom message instead, but I can't make the error disappear.

m-stanley commented 6 years ago

Sorry, I don't think I was clear.

I'm not advocating removing the error completely - just changing the call to stop that raises the error. When you use stopifnot, it calls stop(..., call. = FALSE) and this is really bad because there's no stack trace.

The only thing you need to change (apart from having your own code to do the checking since you wouldn't be using stopifnot any more) is to call stop(..., call. = TRUE) which is the default.

Honestly I couldn't tell you why the designer of stopifnot chose to remove the stack trace from the error that it raises, or chose not to give the user the option of including a stack trace if they wanted, but they didn't, so dealing with errors from stopifnot as a user is very challenging.

jeroen commented 6 years ago

Have you ever tried the traceback() function in R after getting an error?

m-stanley commented 6 years ago

traceback gets part of the way there, but if I want to use an error handler I have no access to the call:

> tryCatch(stopifnot(FALSE), error = function(x) str(x))
List of 2
 $ message: chr "FALSE is not TRUE"
 $ call   : NULL
 - attr(*, "class")= chr [1:3] "simpleError" "error" "condition"
> tryCatch(stopifnot(FALSE), error = function(x) .traceback())
NULL

So that method works, but only in interactive mode. If I need the call in a condition handler, I won't have it.

jeroen commented 6 years ago

OK fair enough. I have just added a custom stopifnot() function to the package. Could you try if this works better for you?

devtools::install_github("jeroen/openssl")