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

catch_cnd() should take class argument #696

Closed
lionel- opened this issue Dec 12, 2018 · 3 comments
Closed

catch_cnd() should take class argument #696

lionel- opened this issue Dec 12, 2018 · 3 comments
Labels

Comments

@lionel-
Copy link
Member

@lionel- lionel- commented Dec 12, 2018

And probably throw an error when a condition of the excepted class failed to be caught, instead of returning NULL. This way it can safely be used in unit tests and other situations where you expect a condition. WDYT @hadley?

@lionel- lionel- added the cnd label Dec 12, 2018
@hadley
Copy link
Member

@hadley hadley commented Dec 13, 2018

Hmmm, I like the current interface. What you're describing seems more like a special purpose function that should be part of testthat.

@lionel-
Copy link
Member Author

@lionel- lionel- commented Dec 13, 2018

How about just taking an optional condition class defaulting to "condition" then? If not caught, return NULL. There's currently no easy way to catch a condition whose class is in a variable. Then the testthat helper can be implemented around catch_cnd().

catch_cnd <- function(expr, class = "condition") {
  stopifnot(is_string(class))
  handler <- set_names(list(identity), class)

  value(
    tryCatch(!!!handler, {
      force(expr)
      return(NULL)
    })
  )
}

It feels like catch_cnd() is not complete in its current state, at least for programming where the caller invariably has to check for the condition type in addition to checking for NULL.

@hadley
Copy link
Member

@hadley hadley commented Dec 13, 2018

Yeah, I like that.

@lionel- lionel- closed this in cb81ab2 Dec 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants