-
Notifications
You must be signed in to change notification settings - Fork 42
External S7 classes #341
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?
External S7 classes #341
Changes from all commits
d4420ac
99ff6f9
b64a2c3
03618f8
45d51b8
9417ed7
be86116
ec4d621
2607bc1
3dea7d8
9bcf578
4a33112
d186490
370c6da
05246fa
ab30c8e
f8096e0
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 |
|---|---|---|
|
|
@@ -113,6 +113,14 @@ new_class <- function( | |
| # Don't check arguments for S7_object | ||
| if (!is.null(parent)) { | ||
| check_can_inherit(parent) | ||
|
|
||
| # Automatically use an external class if appropriate | ||
| if (!is.null(package) && is_class(parent)) { | ||
| if (!is.null(parent@package) && !identical(parent@package, package)) { | ||
| parent <- new_external_class(parent@package, parent@name) | ||
| } | ||
| } | ||
|
|
||
| if (!is.null(package)) { | ||
| check_name(package) | ||
| } | ||
|
|
@@ -128,30 +136,44 @@ new_class <- function( | |
| } | ||
|
|
||
| # Combine properties from parent, overriding as needed | ||
|
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. This comment may be better placed inside |
||
| all_props <- attr(parent, "properties", exact = TRUE) %||% list() | ||
| new_props <- as_properties(properties) | ||
| all_props[names(new_props)] <- new_props | ||
|
|
||
| if (is.null(constructor)) { | ||
| constructor <- new_constructor(parent, all_props) | ||
| if (is_external_class(parent)) { | ||
| # TODO: cache this so only computed once per session | ||
| properties <- function() { | ||
| inherit_properties(parent$constructor_fun(), new_props) | ||
| } | ||
| constructor <- constructor %||% new_dynamic_constructor(parent, properties) | ||
| } else { | ||
| my_props <- inherit_properties(parent, new_props) | ||
| properties <- function() { | ||
| my_props | ||
| } | ||
| constructor <- constructor %||% new_constructor(parent, my_props) | ||
| } | ||
|
|
||
| object <- constructor | ||
| # Must synchronise with prop_names | ||
| attr(object, "name") <- name | ||
| attr(object, "parent") <- parent | ||
| attr(object, "package") <- package | ||
| attr(object, "properties") <- all_props | ||
| attr(object, "properties") <- properties | ||
| attr(object, "abstract") <- abstract | ||
| attr(object, "constructor") <- constructor | ||
| attr(object, "validator") <- validator | ||
| class(object) <- c("S7_class", "S7_object") | ||
|
|
||
| global_variables(names(all_props)) | ||
| global_variables(names(properties())) | ||
| object | ||
| } | ||
| globalVariables(c("name", "parent", "package", "properties", "abstract", "constructor", "validator")) | ||
|
|
||
| inherit_properties <- function(parent, new) { | ||
| properties <- attr(parent, "properties", exact = TRUE) %||% function() list() | ||
| properties <- properties() | ||
| properties[names(new)] <- new | ||
| properties | ||
| } | ||
|
|
||
| #' @rawNamespace if (getRversion() >= "4.3.0") S3method(nameOfClass, S7_class, S7_class_name) | ||
| S7_class_name <- function(x) { | ||
| paste(c(x@package, x@name), collapse = "::") | ||
|
|
@@ -170,7 +192,7 @@ check_S7_constructor <- function(constructor) { | |
|
|
||
| #' @export | ||
| print.S7_class <- function(x, ...) { | ||
| props <- x@properties | ||
| props <- x@properties() | ||
| if (length(props) > 0) { | ||
| prop_names <- format(names(props)) | ||
| prop_types <- format(vcapply(props, function(x) class_desc(x$class))) | ||
|
|
@@ -217,7 +239,9 @@ c.S7_class <- function(...) { | |
| stop(msg, call. = FALSE) | ||
| } | ||
|
|
||
| can_inherit <- function(x) is_base_class(x) || is_S3_class(x) || is_class(x) | ||
| can_inherit <- function(x) { | ||
| is_base_class(x) || is_S3_class(x) || is_class(x) || is_external_class(x) | ||
| } | ||
|
|
||
| check_can_inherit <- function(x, arg = deparse(substitute(x))) { | ||
| if (!can_inherit(x)) { | ||
|
|
@@ -243,7 +267,8 @@ is_class <- function(x) inherits(x, "S7_class") | |
| #' @rdname new_class | ||
| #' @export | ||
| new_object <- function(.parent, ...) { | ||
| class <- sys.function(-1) | ||
| class <- sys.function(sys.parent()) | ||
|
|
||
| if (!inherits(class, "S7_class")) { | ||
| stop("`new_object()` must be called from within a constructor") | ||
| } | ||
|
|
@@ -268,8 +293,9 @@ new_object <- function(.parent, ...) { | |
| # We have to fill in missing values after setting the initial properties, | ||
| # because custom setters might set property values | ||
| missing_props <- setdiff(nms, union(supplied_props, names(attributes(object)))) | ||
| properties <- class@properties() | ||
| for (prop in missing_props) { | ||
| prop(object, prop, check = FALSE) <- prop_default(class@properties[[prop]]) | ||
| prop(object, prop, check = FALSE) <- prop_default(properties[[prop]]) | ||
| } | ||
|
|
||
| # Don't need to validate if parent class already validated, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,30 @@ new_constructor <- function(parent, properties) { | |
| new_function(args, body, env) | ||
| } | ||
|
|
||
| new_dynamic_constructor <- function(parent, properties_fun) { | ||
| constructor_fun <- parent$constructor_fun | ||
| force(properties_fun) | ||
|
|
||
| function(...) { | ||
| parent_class <- constructor_fun() | ||
| args_info <- constructor_args(parent_class, properties_fun()) | ||
|
|
||
| args <- list(...) | ||
| parent_args <- dynamic_args(args, args_info$parent) | ||
| self_args <- dynamic_args(args, args_info$self) | ||
|
|
||
| parent_obj <- do.call("parent_class", parent_args) | ||
| do.call("new_object", c(list(parent_obj), self_args)) | ||
| } | ||
| } | ||
| dynamic_args <- function(args, selected) { | ||
| missing <- setdiff(selected, names(args)) | ||
|
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. It took me a few minutes to figure this out, maybe add a comment like # Pad `args` with currently missing arguments |
||
| args[missing] <- missing_args(missing) | ||
|
|
||
| args[selected] | ||
| } | ||
|
|
||
|
|
||
| constructor_args <- function(parent, properties = list()) { | ||
| parent_args <- names2(formals(class_constructor(parent))) | ||
|
|
||
|
|
@@ -52,7 +76,7 @@ constructor_args <- function(parent, properties = list()) { | |
| if (is_class(parent) && !parent@abstract) { | ||
| # Remove any parent properties; can't use parent_args() since the constructor | ||
| # might automatically set some properties. | ||
| self_args <- setdiff(self_args, names2(parent@properties)) | ||
| self_args <- setdiff(self_args, names2(parent@properties())) | ||
| } | ||
|
|
||
| list( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,72 @@ | ||||||
| #' Classes from other packages | ||||||
| #' | ||||||
| #' @description | ||||||
| #' You need an explicit external class when you want extend a class defined in | ||||||
|
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.
Suggested change
|
||||||
| #' another package. An external class ensures that the class definition from | ||||||
| #' the other package is not literally inlined in your package, ensuring that | ||||||
| #' when the other package changes your package doesn't need to be rebuilt to | ||||||
| #' get those changes. | ||||||
| #' | ||||||
| #' [new_class()] will automatically convert an S7 class to an external class | ||||||
|
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. It automatically converts the parent right? |
||||||
| #' if its `package` property has been set, and it's different to the `package` | ||||||
| #' of the subclass. This should ensure that extending a class in another package | ||||||
| #' just works without you having to do anything extra. | ||||||
| #' | ||||||
| #' Extending a class creates a hard requirement on the package that defines it; | ||||||
| #' i.e. you must list the package in the `Imports` field in your package's | ||||||
| #' `DESCRIPTION`. | ||||||
| #' | ||||||
| #' @param package Package the class is defined in. | ||||||
| #' @param name Name of class, as a string. | ||||||
| #' @param constructor_fun A zero-argument function that yields the constructor | ||||||
| #' of the external class. For expert use only. | ||||||
| #' @returns An S7 external class, i.e. a list with class | ||||||
| #' `S7_external_class`. | ||||||
| #' @export | ||||||
| #' @examples | ||||||
| #' foo <- new_class("foo", properties = list(x = class_integer)) | ||||||
| #' foo_ex <- new_external_class("S7", "foo", function() foo) | ||||||
| #' | ||||||
| #' foo2 <- new_class("foo", parent = foo_ex) | ||||||
| #' foo2() | ||||||
| new_external_class <- function(package, | ||||||
| name, | ||||||
| constructor_fun = NULL) { | ||||||
|
|
||||||
| check_name(package) | ||||||
| check_name(name) | ||||||
|
|
||||||
| if (is.null(constructor_fun)) { | ||||||
| constructor_fun <- function() getExportedValue(package, name) | ||||||
| } else { | ||||||
| check_function(constructor_fun, alist()) | ||||||
| } | ||||||
|
|
||||||
| constructor <- constructor_fun() | ||||||
| if (!is_class(constructor)) { | ||||||
| stop("`constructor_fun()` must yield an S7 class") | ||||||
|
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. This might be a fairly esoteric error if you don't actually provide
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.
Suggested change
|
||||||
| } | ||||||
|
|
||||||
| out <- list( | ||||||
| package = package, | ||||||
| name = name, | ||||||
| constructor_fun = constructor_fun | ||||||
| ) | ||||||
| class(out) <- "S7_external_class" | ||||||
| out | ||||||
| } | ||||||
|
|
||||||
| #' @export | ||||||
| print.S7_external_class <- function(x, ...) { | ||||||
| cat( | ||||||
| "<S7_external_class> ", | ||||||
| x$package, "::", x$name, "\n", | ||||||
| sep = "" | ||||||
| ) | ||||||
| invisible(x) | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
| is_external_class <- function(x) { | ||||||
| inherits(x, "S7_external_class") | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -186,7 +186,8 @@ prop_val <- function(object, name) { | |
| # Get underlying property object from class | ||
| prop_obj <- function(object, name) { | ||
| class <- S7_class(object) | ||
| attr(class, "properties")[[name]] | ||
| if (is.null(class)) return() | ||
| attr(class, "properties")()[[name]] | ||
| } | ||
|
|
||
| #' @rdname prop | ||
|
|
@@ -316,7 +317,7 @@ prop_names <- function(object) { | |
| c("name", "parent", "package", "properties", "abstract", "constructor", "validator") | ||
| } else { | ||
| class <- S7_class(object) | ||
| props <- attr(class, "properties", exact = TRUE) | ||
| props <- attr(class, "properties", exact = TRUE)() | ||
|
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. Is it worth adding |
||
| if (length(props) == 0) { | ||
| character() | ||
| } else { | ||
|
|
||
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.
Does it need to be a nested if?