-
Notifications
You must be signed in to change notification settings - Fork 44
Silently ignore missing arguments in constructor #446
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
2985a9d
57dabec
16cb422
8bdad50
11aaf2d
438ae21
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| #define R_NO_REMAP | ||
| #include <R.h> | ||
| #include <Rinternals.h> | ||
|
|
||
| SEXP collect_dots_skip_missing_(SEXP env, SEXP list_dddExprs_call) { | ||
| // This function is equivalent to `base::list(...)`, except it | ||
| // silently skips missing arguments. Ideally we could iterate | ||
| // over the DOTSXP list of promises directly, but there is currently | ||
| // no non-"non-API" way to do this. So we use `base::missing(..i)` to | ||
| // test for missingness, and use `substitute(list(...))` to get the | ||
| // promise expressions. | ||
| static SEXP missing_call = NULL; | ||
| if (missing_call == NULL) { | ||
| SEXP missing_fun = Rf_eval(Rf_install("missing"), R_BaseEnv); | ||
| missing_call = Rf_lang2(missing_fun, R_NilValue); | ||
| R_PreserveObject(missing_call); | ||
| } | ||
| // 14 = 2 for ".." + up to 10 digit number + '\0' + 1 extra for safety | ||
| static char ddi_buf[14] = ".."; | ||
| static char *i_buf = ddi_buf + 2; | ||
| ddi_buf[13] = '\0'; // Technically not necessary, but just to be safe | ||
|
|
||
| PROTECT_INDEX pi; | ||
| PROTECT_WITH_INDEX(R_NilValue, &pi); | ||
|
|
||
| { | ||
| unsigned int i = 1; | ||
| SEXP prev_node = list_dddExprs_call; | ||
| SEXP ddExpr_node = CDR(list_dddExprs_call); | ||
| for (; ddExpr_node != R_NilValue; i++) { | ||
| snprintf(i_buf, sizeof(ddi_buf) - 2, "%u", i); | ||
| SEXP ddSym = Rf_install(ddi_buf); | ||
| SETCADR(missing_call, ddSym); | ||
| SEXP is_missing = Rf_eval(missing_call, env); | ||
| REPROTECT(is_missing, pi); | ||
|
|
||
| if (Rf_asLogical(is_missing)) { | ||
| ddExpr_node = CDR(ddExpr_node); | ||
| SETCDR(prev_node, ddExpr_node); | ||
| } else { | ||
| if (TAG(ddExpr_node) == R_NilValue) { | ||
| SEXP val_expr = CAR(ddExpr_node); | ||
| if (TYPEOF(val_expr) == SYMSXP) { | ||
| SET_TAG(ddExpr_node, val_expr); | ||
| } | ||
| } | ||
| SETCAR(ddExpr_node, ddSym); | ||
| prev_node = ddExpr_node; | ||
| ddExpr_node = CDR(ddExpr_node); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| UNPROTECT(1); // is_missing | ||
| return Rf_eval(list_dddExprs_call, env); | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -69,3 +69,23 @@ | |||||
| new_object(foo(...), y = y) | ||||||
| <environment: 0x0> | ||||||
|
|
||||||
| # can create constructors with missing or lazy defaults | ||||||
|
|
||||||
| Code | ||||||
| Person() | ||||||
| Condition | ||||||
| Error: | ||||||
| ! <Person> object properties are invalid: | ||||||
| - @first_name must be <character>, not <NULL> | ||||||
| - @last_name must be <MISSING> or <character>, not <NULL> | ||||||
| - @nick_name must be <character>, not <NULL> | ||||||
|
|
||||||
| --- | ||||||
|
|
||||||
| Code | ||||||
| Person("Alice") | ||||||
| Condition | ||||||
| Error: | ||||||
| ! <Person> object properties are invalid: | ||||||
| - @last_name must be <MISSING> or <character>, not <NULL> | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems a bit confusing because it is missing?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue happens because This whole test probably needs to be rewritten to make sense with the new behavior.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lines 260 to 261 in 15a01a3
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's not set, should we be using
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To do that, we would have to use
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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may also want to add an new_property(..., initializer = setter)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we just use
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: |
||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.