Conversation
… ie we check the actual sys.parent(), not just -1L
…n S7 extension of S4
…S4 object is properly initialized; assumes that the inherited initialize() method has the same semantics as the default one
…ith related cleanup
hadley
left a comment
hadley
left a comment
There was a problem hiding this comment.
Here are a bunch of minor style suggestions; feel free to take as many or as few as you like.
| #' @export | ||
| new_object <- function(`_parent`, ...) { | ||
| class <- sys.function(-1) | ||
| class <- sys.function(sys.parent()) |
There was a problem hiding this comment.
Can you explain why this is needed/what it does?
There was a problem hiding this comment.
The case that exposed this problem is initialize(new_object()), which causes new_object() to be evaluated when forced by initialize(). As you know, this results in a confusing stack trace, where new_object() is nested within initialize(). That makes sys.function(-1) see initialize(), while sys.parent() returns the actual index of the frame in which new_object() is evaluated.
To quote the admittedly vague man page:
The parent frame of a function evaluation is the environment in
which the function was called. It is not necessarily numbered one
less than the frame number of the current evaluation,
I guess this is partly why rlang was created :)
| overridden <- intersect(names(child_props), names(parent_props)) | ||
| parent_S4 <- if (is_S4_class(parent)) parent else S4_ancestor(parent) | ||
| if (!is.null(parent_S4)) { | ||
| check_S4_slot_overrides(child_props, parent_S4, call = call) |
There was a problem hiding this comment.
Given that parent_S4 is only used by check_S4_slot_overrides(), maybe pass parent and then early return if it's an S4 class?
| } else { | ||
| quote(S7::S7_object()) | ||
| } | ||
| new_object_call <- |
There was a problem hiding this comment.
I think it would be worth refactoring this if statement to make it clear that there are now 3 cases?
There was a problem hiding this comment.
Except that the third case depends on the first two cases being resolved, since new_object_call is being wrapped.
There was a problem hiding this comment.
Would it be clearer to rename this envir instead of creating a S4_envir?
|
|
||
| prop_is_dynamic <- function(prop) is.function(prop$getter) | ||
|
|
||
| prop_is_encapsulated <- function(prop) { |
|
|
||
| S4PropertyS7Parent := new_class(package = NULL) | ||
| S4_register(S4PropertyS7Parent) | ||
| setClass( |
There was a problem hiding this comment.
Use local_S7_class() here and elsewhere?
There was a problem hiding this comment.
I'll make a separate PR addressing the S4 class clean up in tests.
| parent = getClass("S4regAccessorParent"), | ||
| package = NULL | ||
| ) | ||
| expect_error( |
There was a problem hiding this comment.
Might be useful to have at least one snapshot test of this error so you can see the whole thing in one place?
Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
2 participants
This PR addresses several high-priority compatibility issues found in #720:
unlist().methods::initialize(). Parent slots were being dropped during construction.new_object()correctly detects its evaluation frame even when lazily evaluated as part of another call.initialize()method on S4-derived classes delegate to any non-default parentinitialize()method, so as not to assume that the custom method has the same interface and semantics as the default method. When S4 is not extending S7, we could just not register ourinitialize()method when the default is overridden by a parent, but this seems like the simplest solution.class_extends()to correctly handle S4/S7 inheritance cases.