Silently ignore missing arguments in constructor#446
Silently ignore missing arguments in constructor#446t-kalinowski wants to merge 6 commits intomainfrom
Conversation
tests/testthat/_snaps/constructor.md
Outdated
| Condition | ||
| Error: | ||
| ! <Person> object properties are invalid: | ||
| - @last_name must be <MISSING> or <character>, not <NULL> |
There was a problem hiding this comment.
This seems a bit confusing because it is missing?
There was a problem hiding this comment.
The issue happens because person@last_name <- <value> is never set since <value> is missing. Then, when the validator fetches person@last_name, it returns NULL (because attr(x, "does_not_exist") return NULL), which is the wrong type and causes the error.
This whole test probably needs to be rewritten to make sense with the new behavior.
There was a problem hiding this comment.
If it's not set, should we be using quote(expr = )?
There was a problem hiding this comment.
To do that, we would have to use Rf_setAttrib(object, Rf_install("name"), R_missingArg) in prop<- when the object is first constructed.
quote(expr=) is such a pain to work with at the R level; I'm beginning to think that perhaps we should not create a pattern that requires all object instances to be handled with care, checking with missing() like this.
To make the "deprecate via getter+setter" pattern possible, maybe we should return to what we discussed in #396 (comment) and altogether bypass or not invoke custom setters on initial construction.
We would then need to provide a convenient way to "opt-in" to running the setters without requiring a full custom constructor.
Perhaps we add an argument to new_class(..., initializer = function(self) {}), a function authors can provide. The initializer function will be called with the output of the constructor after the constructor() has returned but before validator() is called. (somewhat analogous to __new__ and __init__ in Python.)
There was a problem hiding this comment.
We may also want to add an initializer arg to new_property, defaulting to setter.
new_property(..., initializer = setter)There was a problem hiding this comment.
Could we just use class_missing as the sentinel value? Or perhaps something similar but named like arg_missing?
There was a problem hiding this comment.
I think we got a bit side-tracked into a much bigger topic than I was thinking. Looking at this with fresh eyes, I think all that I want is for the error message to be:
! <Person> object properties are invalid:
- @last_name must be <character>, not <NULL>
|
If/when promise accessors become part of the C "API", the code in this PR should be updated to use them (posit-dev/ark#538 (comment)) |
As discussed in #445, this PR allows missing arguments in a constructor to "skip" setting the property and avoid invoking a custom setter.