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

try() wrapper #120

Closed
hadley opened this issue Nov 5, 2015 · 15 comments
Closed

try() wrapper #120

hadley opened this issue Nov 5, 2015 · 15 comments

Comments

@hadley
Copy link
Member

@hadley hadley commented Nov 5, 2015

Maybe maybe()? Should it be a wrapper around functions or around expressions?

library(purrr)
maybe1 <- function(expr, otherwise, quiet = TRUE) { 

  if (missing(otherwise)) { 
    try(expr, silent = quiet)
  } else {
    tryCatch(expr,
      error = function(e) {
        if (!quiet)
          message("Error: ", e$message)
        otherwise
      }
    )
  }
}

maybe2 <- function(f, otherwise, quiet = TRUE) {
  if (missing(otherwise)) { 
    function(...) {
      try(f(...), silent = quiet)  
    }
  } else {
    function(...) {
      tryCatch(f(...),
        error = function(e) {
          if (!quiet)
            message("Error: ", e$message)
          otherwise
        }
      )
    }
  }
}


maybe1(log("a"))
maybe1(log("a"), NULL)

maybe2(log)("a")
maybe2(log, NULL)("a")

"a" %>% map(~ maybe1(log(.)))
"a" %>% map(maybe2(log))

cc @jennybc, @lionel-

@lionel-
Copy link
Member

@lionel- lionel- commented Nov 5, 2015

The first one feels more like traditional "throw/catch" error handling, while the second feels more functional, and can be composed with other function operators. So I prefer the second form.

Loading

@jennybc
Copy link
Member

@jennybc jennybc commented Nov 6, 2015

I like the second one but for a different reason. I am less comfortable with expressions and it gives me the option to define the function, then use it with map().

safe_log <- maybe2(log)
list("a", exp(1)) %>% map(safe_log)

On a slight tangent, I am repeatedly surprised that try() silences fatal errors and still lets warnings through. It feels like it should work more like logging levels. Would you ever consider a version where quiet suppresses warnings?

## how can this be silent?
maybe2(log)("a")
## and this be not?
maybe2(log)(-1)
#> Warning in f(...): NaNs produced
#> [1] NaN

Loading

@lionel-
Copy link
Member

@lionel- lionel- commented Nov 6, 2015

or have another function operator, silently()? It would fail on errors but suppress messages and warnings.

I am repeatedly surprised that try() silences fatal errors and still lets warnings through.

The problem is that you may not know a warning went through then. That's a big difference with errors which show up as either a "try-error" object or as a user-specified default value. It is a bit dangerous to silence warnings completely so it should be really explicit that it's what the user wants. If we add this option to maybe(), it should probably be another argument, such as silent = FALSE (which could call silently() on the resulting function).

Loading

@hadley
Copy link
Member Author

@hadley hadley commented Nov 6, 2015

Another option is to bring in something like testthat::evaluate_promise(). It's a terrible name but the idea is good - it basically turns all side effects into outputs, so you get a list of result, output, messages and warnings (errors are passed through unchanged).

I'd kind of prefer try() to work like that - instead of sometimes returning a try-error and sometimes returning something else, I'd prefer it to always returning a list with components result and error (and one would always be empty)

Loading

@hadley
Copy link
Member Author

@hadley hadley commented Nov 6, 2015

Oooh, I rather like that:

library(purrr)
safe <- function(f, quiet = TRUE) {
  show_error <- function(e) {
    if (quiet) return()

  }
  function(...) {
    tryCatch(list(result = f(...), error = NULL),
      error = function(e) {
        if (!quiet)
          message("Error: ", e$message)

        list(result = NULL, error = e)
      }
    )
  }
}

safe_log <- safe(log)
out <- list("a", exp(1)) %>% map(safe_log)
zip_n(out)

Especially since it works so naturally with zip.

I'm not sure if safe() is the right name, but it does seem a bit different to maybe(). I'm also not sure if we need both safe() and maybe()

Loading

@hadley
Copy link
Member Author

@hadley hadley commented Nov 6, 2015

Are there other side-effecty things that we could also capture this way?

Loading

@jennybc
Copy link
Member

@jennybc jennybc commented Nov 6, 2015

The safe() above is great (and a 💡just went on re: what zip_n() is good for).

Could quiet work like options(warn=2)? So warnings become errors, both are silenced, and get duly recorded in error?

Loading

@hadley
Copy link
Member Author

@hadley hadley commented Nov 6, 2015

@jennybc would you prefer that to having warnings stored separately in their own warnings slot?

Loading

@jennybc
Copy link
Member

@jennybc jennybc commented Nov 6, 2015

I don't feel qualified to say. I suppose it's possible to get both a warning and an error?

Loading

@lionel-
Copy link
Member

@lionel- lionel- commented Nov 7, 2015

safe() is a nice idea. How about calling it something along the lines of effects()? Since it returns a list of all effects.

I think there's room for maybe(), silently() and effects() as each has its own purpose.

Are there other side-effecty things that we could also capture this way?

Can we capture printed outputs issued by cat()?

Loading

@lionel-
Copy link
Member

@lionel- lionel- commented Nov 7, 2015

How about calling it something along the lines of effects()? Since it returns a list of all effects.

Though it doesn't capture assignments, so it won't be about all effects.

Loading

@hadley
Copy link
Member Author

@hadley hadley commented Nov 9, 2015

We can only capture "output" effects, so maybe outputs()?

We can capture:

  • printed output to console
  • messages
  • warnings
  • (one) error

The question to me is whether this should be one function with an optional argument to capture errors, or two functions, one regular outputs and one for errors. I think we'd still want maybe() for the cases where you don't care about the error, you just want to replace it with a default value.

Loading

hadley added a commit that referenced this issue Nov 10, 2015
@hadley
Copy link
Member Author

@hadley hadley commented Nov 10, 2015

Checked in a first pass, mostly for discussion. I'm not confident about the names or the exact breakdown of tasks between functions.

Loading

@hadley
Copy link
Member Author

@hadley hadley commented Nov 10, 2015

Do we also need succeeded() and failed() helper functions? Should the results from safe() and output() have a class and default print method?

Loading

@hadley hadley closed this in 6a263a3 Nov 12, 2015
@hadley
Copy link
Member Author

@hadley hadley commented Nov 12, 2015

@wch and I came up with the idea to adverbs, so now we have safely(), quietly() and possibly(). I like these names, but welcome more feedback.

Loading

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

Successfully merging a pull request may close this issue.

None yet
3 participants