smbache / ensurer

Ensure values are as expected at runtime
Other
84 stars 4 forks source link

Custom error message #5

Closed gaborcsardi closed 9 years ago

gaborcsardi commented 9 years ago

I guess this is possible with fail_with, but it seems rather cumbersome. When I do something like

ensure_that(x, attr %in% names(attributes(.)))

the error message looks as

Error:  The following condition failed:
     * attr %in% names(attributes(.))

and I would rather like something like

Error:  The following condition failed:
     * has no attribute 'name'
smbache commented 9 years ago

I see two options, but would need to change a bit:

Have string arguments define the msg for the previous condition, say

something %>% 
    ensure_that(attr %in% names(attributes(.)), "has no attribute 'name'")

Or have fail_with and err_desc be regular arguments, and use named argumens for individual messages, e.g.

something %>% 
    ensure_that(`has no attribute 'name'` = attr %in% names(attributes(.)))
gaborcsardi commented 9 years ago

I don't like the second one, because I might want to include something about the object (or the check) in the message, e.g. the name of the attribute being checked, and then it is cumbersome to create the names and use do.call, etc.

The first one is better imo. How about making it more explicit:

something %>% 
    ensure_that(attr %in% names(attributes(.)) %or% "has no attribute 'name'")

This is also easier to implement, I think, no special evaluation is needed (for this).

gaborcsardi commented 9 years ago

Actually %or% is not good, a separate argument is better, because you often have multiple conditions with an &&, and then %or% messes up the precedence.

smbache commented 9 years ago

Yeah, glad you too get confused about what is best; both this and the other issue would have been dealt with if only I cod decide on something and be happy with it...

smbache commented 9 years ago

Maybe ~ could be used in place of %or%. Would that be satisfactory?

gaborcsardi commented 9 years ago

Yeah, maybe ~ is good for precedence, but it does not improve the readability much or at all. So then you might as well go with the separate argument imo.

Btw. I think we (I) just need to start using the package and write checks, I need them anyway. Then these issues come up and we can fix them.

smbache commented 9 years ago

Just made a commit where you can use the ~: it was way easier to deal with, also given the other issues I tried to fix today. This works, and will also carry over to contracts using existing contracts.

Example:

> A <- ensures_that(!is.data.frame(.) ~ "Value is a data.frame")
> iris %>% A
 Error: conditions failed for call 'iris %>% A':
     * Value is a data.frame

> B <- ensures_that(!is.null(.), +A)
> B
Ensures that
    * !is.null(.) 
    * !is.data.frame(.) 

> iris %>% B
 Error: conditions failed for call 'iris %>% B':
     * Value is a data.frame 
gaborcsardi commented 9 years ago

Very cool, will try all the new things later today!

gaborcsardi commented 9 years ago

OK, here are my non-comprehensive tests so far:

ensure_has_attr <- function(x, attr) {
  ensure_that(x, attr %in% names(attributes(.)) ~ "has attribute attr")
}
ensure_has_attr(1, "foo")
#> Error: conditions failed for call 'ensure_has_attr(1, "foo")':
#>   * has attribute attr
ensure_has_attr(1, attr="foo")
#> Error: conditions failed for call 'ensure_has_attr(1, attr = "foo")':
#>   * has attribute attr

Good.

However, for this I would expect an error for ensure_that itself:

ensure_has_attr2 <- function(x, attr) {
  ensure_that(x, attr %in% names(attributes(.)) ~ "has attribute attr" + 10)
}
ensure_has_attr2(1, "foo")
#> Error: conditions failed for call 'ensure_has_attr2(1, "foo")':
#>   * attr %in% names(attributes(.))

Plus, I cannot seem to create the message when the test is actually performed:

ensure_has_attr3 <- function(x, attr) {
  ensure_that(x,
    attr %in% names(attributes(.)) ~ sprintf("has attribute '%s'", attr))
}
ensure_has_attr3(1, "foo")
#> Error: conditions failed for call 'ensure_has_attr3(1, "foo")':
#>   * attr %in% names(attributes(.))

So I guess the ideal solution would be to save the expression after the ~, and then evaluate it when the check is performed. If it evaluates with an error, then give a "regular" error message for ensure_that, otherwise use the string that was just created as the ensure_that error message.

smbache commented 9 years ago

Of course the custom message needs to be evaluated. Try now :)

gaborcsardi commented 9 years ago

OK, it seems that all is good now.