-
Notifications
You must be signed in to change notification settings - Fork 37
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
Silently ignore missing arguments in constructor #446
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit confusing because it is missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 260 to 261 in 15a01a3
// This is commented out because we currently have no way to distinguish between | |
// a prop with a value of NULL, and a prop value that is unset/missing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not set, should we be using quote(expr = )
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.