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

Silently ignore missing arguments in constructor #446

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

t-kalinowski
Copy link
Member

@t-kalinowski t-kalinowski commented Sep 23, 2024

As discussed in #445, this PR allows missing arguments in a constructor to "skip" setting the property and avoid invoking a custom setter.

Condition
Error:
! <Person> object properties are invalid:
- @last_name must be <MISSING> or <character>, not <NULL>
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

S7/src/prop.c

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.

Copy link
Member

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 = )?

Copy link
Member Author

@t-kalinowski t-kalinowski Sep 24, 2024

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.)

Copy link
Member Author

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)

Copy link
Collaborator

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?

Copy link
Member

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>

R/utils.R Show resolved Hide resolved
@t-kalinowski
Copy link
Member Author

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))

@t-kalinowski t-kalinowski marked this pull request as draft October 1, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants