Validation improvements #149
Conversation
And improve docs
And add test
|
@DavisVaughan can you please review? Probably easiest to start with the documentation improvements to |
| if (isTRUE(check) && !class_inherits(value, prop$class)) { | ||
| stop(sprintf("%s@%s must be of class %s, not %s", | ||
| obj_desc(object), name, | ||
| class_desc(prop$class), | ||
| obj_desc(value) | ||
| ), call. = FALSE) | ||
| stop(prop_error_type(object, name, prop$class, value), call. = FALSE) |
There was a problem hiding this comment.
Could you extract out a validate_property() from validate_properties() and then use that here? Then if we add more validation checks we don't have to keep these in sync
There was a problem hiding this comment.
I thought about that too. It's just pretty marginal since it's only used in two places and the logic is v simple.
R/valid.R
Outdated
| #' automatically when creating new objects (at the end of [new_object]) and | ||
| #' when setting any property. | ||
| #' @description | ||
| #' `validate()` calls the validator of an R7 object. This is done automatically |
There was a problem hiding this comment.
My first two thoughts were:
- What is the job of a validator? Like what exactly is the scope of what it should be validating
- Where do I set a validator? (Looking into the examples you learn that you set this in
new_class(validator=)but it might be nice to mention that here somehow)
There was a problem hiding this comment.
I've expanded the validator docs in new_class() and linked to them from here.
| #' r | ||
| #' } | ||
| #' rightwards(r, 20) | ||
| validate <- function(object, properties = TRUE) { |
There was a problem hiding this comment.
If I were designing this function, this is where I'd put ... to force named arguments, because validate(x, FALSE) is essentially meaningless to me without the argument name.
Plus this feels like a function which has a decent chance that we will add more arguments to it, and that would free us from caring about argument order.
Not sure how we feel about this principle for R7 though, since it will be tied closely to base R
There was a problem hiding this comment.
Yeah, agreed with both points. But since this is going in base R (which doesn't use that pattern), and properties is mostly their for internal use, I'll leave as is.
There was a problem hiding this comment.
Adding an unneeded ... to the argument list also comes with a price. I would never want to pay that just to force users..
There was a problem hiding this comment.
Explicit formal arguments also improve code clarity. Users are free to follow whatever calling convention they like.
| if (is.null(validator)) { | ||
| return(invisible(object)) | ||
| # First, check property types - if these are incorrect, the validator | ||
| # is likely to return spurious errors |
There was a problem hiding this comment.
If the errors in the object validator are likely to be spurious, should we just go ahead and error immediately if any properties are invalid? Maybe it could have a header of Invalid <obj> properties:?
There was a problem hiding this comment.
Oh I see you've tried to do something like this by avoiding the object validator with the length(errors) == 0 branch. Hmm, I think I still prefer an immediate error - but only because I would mention in the header that it was the properties that were invalid (I don't feel strongly about this if you want to keep it as is)
Co-authored-by: Davis Vaughan <davis@rstudio.com>
| #' | ||
| #' The job of a validator is to determine whether the object is valid, | ||
| #' i.e. if the current property values form an allowed combination. The | ||
| #' types of the properties are always automatically validator so the job of |
There was a problem hiding this comment.
always automatically validator
typo here, validator->valid
validate()checks properties types, then recursively checks object