From c1a6ac2fd4092449caaef1d9ae0ac6ef79b9424e Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 19 Sep 2024 15:01:09 -0400 Subject: [PATCH 01/19] include dynamic settable props in the default constructor signature --- R/constructor.R | 5 +++-- R/property.R | 4 ++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/R/constructor.R b/R/constructor.R index fb24231d..d7572bbd 100644 --- a/R/constructor.R +++ b/R/constructor.R @@ -48,9 +48,10 @@ new_constructor <- function(parent, properties) { constructor_args <- function(parent, properties = list()) { parent_args <- formals(class_constructor(parent)) + # Remove read-only properties + properties <- properties[!vlapply(properties, prop_is_read_only)] + self_arg_nms <- names2(properties) - # Remove dynamic arguments - self_arg_nms <- self_arg_nms[vlapply(properties, function(x) is.null(x$getter))] if (is_class(parent) && !parent@abstract) { # Remove any parent properties; can't use parent_args() since the constructor diff --git a/R/property.R b/R/property.R index 2bf677ec..75c78eef 100644 --- a/R/property.R +++ b/R/property.R @@ -479,3 +479,7 @@ as_property <- function(x, name, i) { new_property(x, name = name) } } + +prop_is_read_only <- function(prop) { + is.function(prop$getter) && !is.function(prop$setter) +} From 4c8c166dd0b0fc5a0c16c5e97ec71c6273dd46b9 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 19 Sep 2024 15:08:10 -0400 Subject: [PATCH 02/19] add tests --- tests/testthat/test-constructor.R | 34 +++++++++++++++++++++++++++++++ tests/testthat/test-property.R | 8 ++++++-- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/tests/testthat/test-constructor.R b/tests/testthat/test-constructor.R index 1809a662..c51efd02 100644 --- a/tests/testthat/test-constructor.R +++ b/tests/testthat/test-constructor.R @@ -158,3 +158,37 @@ test_that("can create constructors with missing or lazy defaults", { expect_error(p@birthdate <- as.Date('1970-01-01'), "Can\'t set read-only property Person@birthdate") }) + + + +test_that("Dynamic settable properties are included in constructor", { + Foo <- new_class( + name = "Foo", + properties = list( + dynamic_settable = new_property( + class_numeric, + getter = function(self) self@dynamic_settable, + setter = function(self, value) { + self@dynamic_settable <- value + self + } + ), + + dynamic_read_only = new_property( + class_numeric, + getter = function(self) 99, + ) + ) + ) + + expect_equal(formals(Foo), pairlist(dynamic_settable = numeric())) + expect_equal(Foo()@dynamic_settable, numeric()) + expect_equal(Foo(3)@dynamic_settable, 3) + + foo <- Foo() + expect_error(foo@dynamic_read_only <- 1, + "Can't set read-only property @dynamic_read_only") + foo@dynamic_settable <- 1 + expect_equal(foo@dynamic_settable, 1) + +}) diff --git a/tests/testthat/test-property.R b/tests/testthat/test-property.R index e244ac83..11fabcc5 100644 --- a/tests/testthat/test-property.R +++ b/tests/testthat/test-property.R @@ -407,8 +407,9 @@ test_that("custom getters don't infinitely recurse", { ) )) + expect_equal(someclass("foo")@someprop, "FOO") x <- someclass() - expect_null(x@someprop) + expect_equal(x@someprop, character()) x@someprop <- "foo" expect_equal(x@someprop, "FOO") @@ -429,8 +430,11 @@ test_that("custom setters can call custom getters", { ) )) + x <- someclass("foo") + expect_equal(x@someprop, "FOO") + x <- someclass() - expect_null(x@someprop) + expect_equal(x@someprop, character()) x@someprop <- "foo" expect_equal(x@someprop, "FOO") From 2629d4aa876fcf452da2bbb5b8c1522e7908592d Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Wed, 25 Sep 2024 08:10:38 -0400 Subject: [PATCH 03/19] Accept `...` as a default --- R/constructor.R | 10 ++++++++++ R/property.R | 25 +++++++++++++++++-------- man/new_property.Rd | 25 +++++++++++++++++-------- 3 files changed, 44 insertions(+), 16 deletions(-) diff --git a/R/constructor.R b/R/constructor.R index d7572bbd..38bab7e9 100644 --- a/R/constructor.R +++ b/R/constructor.R @@ -64,6 +64,12 @@ constructor_args <- function(parent, properties = list()) { function(name) prop_default(properties[[name]])) ) + is_dots <- vlapply(self_args, identical, quote(...)) + if (any(is_dots)) { + self_args[is_dots] <- NULL + append(self_args, after = which.max(is_dots)-1) <- alist(... = ) + } + list(parent = parent_args, self = self_args) } @@ -86,3 +92,7 @@ as_names <- function(x, named = FALSE) { } lapply(x, as.name) } + +`append<-` <- function(x, after = length(x), value) { + append(x, value, after) +} diff --git a/R/property.R b/R/property.R index 75c78eef..a2c1e185 100644 --- a/R/property.R +++ b/R/property.R @@ -33,9 +33,11 @@ #' your code can assume that `value` has known type. #' @param default When an object is created and the property is not supplied, #' what should it default to? If `NULL`, it defaults to the "empty" instance -#' of `class`. This can also be a quoted call, which then becomes a standard -#' function promise in the default constructor, evaluated at the time the -#' object is constructed. +#' of `class`. Quoted calls become standard function argument promises in the +#' default constructor, evaluated at the time the object is constructed. A +#' value of `quote(...)` indicates that the property is not a named argument +#' in the constructor, and it is not set unless explicitly supplied. The +#' default value for the unset property in that case is `NULL`. #' @param name Property name, primarily used for error messages. Generally #' don't need to set this here, as it's more convenient to supply as a #' the element name when defining a list of properties. If both `name` @@ -71,7 +73,12 @@ #' args(clock) #' #' # These can be useful if you want to deprecate a property -#' person <- new_class("person", properties = list( +#' # For example, say, at first you define +#' Person <- new_class("Person", properties = list( +#' firstName = class_character +#' )) +#' # Then, to deprecate `firstName` and rename it to `first_name` +#' Person <- new_class("Person", properties = list( #' first_name = class_character, #' firstName = new_property( #' getter = function(self) { @@ -82,12 +89,14 @@ #' warning("@firstName is deprecated; please use @first_name instead", call. = FALSE) #' self@first_name <- value #' self -#' } +#' }, +#' default = quote(...) #' ) #' )) -#' hadley <- person(first_name = "Hadley") -#' hadley@firstName -#' hadley@firstName <- "John" +#' hadley <- Person(firstName = "Hadley") # warning +#' hadley <- Person(first_name = "Hadley") +#' hadley@firstName # warning +#' hadley@firstName <- "John" # warning #' hadley@first_name #' #' # Properties can have default values that are quoted calls. diff --git a/man/new_property.Rd b/man/new_property.Rd index 461e64b0..39f40e09 100644 --- a/man/new_property.Rd +++ b/man/new_property.Rd @@ -41,9 +41,11 @@ your code can assume that \code{value} has known type.} \item{default}{When an object is created and the property is not supplied, what should it default to? If \code{NULL}, it defaults to the "empty" instance -of \code{class}. This can also be a quoted call, which then becomes a standard -function promise in the default constructor, evaluated at the time the -object is constructed.} +of \code{class}. Quoted calls become standard function argument promises in the +default constructor, evaluated at the time the object is constructed. A +value of \code{quote(...)} indicates that the property is not a named argument +in the constructor, and it is not set unless explicitly supplied. The +default value for the unset property in that case is \code{NULL}.} \item{name}{Property name, primarily used for error messages. Generally don't need to set this here, as it's more convenient to supply as a @@ -92,7 +94,12 @@ try(clock(now = 10)) args(clock) # These can be useful if you want to deprecate a property -person <- new_class("person", properties = list( +# For example, say, at first you define +Person <- new_class("Person", properties = list( + firstName = class_character +)) +# Then, to deprecate `firstName` and rename it to `first_name` +Person <- new_class("Person", properties = list( first_name = class_character, firstName = new_property( getter = function(self) { @@ -103,12 +110,14 @@ person <- new_class("person", properties = list( warning("@firstName is deprecated; please use @first_name instead", call. = FALSE) self@first_name <- value self - } + }, + default = quote(...) ) )) -hadley <- person(first_name = "Hadley") -hadley@firstName -hadley@firstName <- "John" +hadley <- Person(firstName = "Hadley") # warning +hadley <- Person(first_name = "Hadley") +hadley@firstName # warning +hadley@firstName <- "John" # warning hadley@first_name # Properties can have default values that are quoted calls. From d02e6007a1dcb3b9738d6081f0f9792593f746e8 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Wed, 25 Sep 2024 11:25:36 -0400 Subject: [PATCH 04/19] Always add `...` to end of formals. --- R/constructor.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/constructor.R b/R/constructor.R index 38bab7e9..0594f595 100644 --- a/R/constructor.R +++ b/R/constructor.R @@ -67,7 +67,7 @@ constructor_args <- function(parent, properties = list()) { is_dots <- vlapply(self_args, identical, quote(...)) if (any(is_dots)) { self_args[is_dots] <- NULL - append(self_args, after = which.max(is_dots)-1) <- alist(... = ) + append(self_args) <- alist(... = ) } list(parent = parent_args, From 1a8f608801721efb028658069bc4eb0033e4c090 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Wed, 25 Sep 2024 11:28:00 -0400 Subject: [PATCH 05/19] Pass along `...` unnamed in the constructor Previously, the generated constructor had: new_object(, ... = ...) It now has: new_object(, ...) --- R/constructor.R | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/R/constructor.R b/R/constructor.R index 0594f595..6f19f9fd 100644 --- a/R/constructor.R +++ b/R/constructor.R @@ -1,7 +1,7 @@ new_constructor <- function(parent, properties) { properties <- as_properties(properties) arg_info <- constructor_args(parent, properties) - self_args <- as_names(names(arg_info$self), named = TRUE) + self_args <- as_names(names(arg_info$self), named = TRUE, unnamed = "...") if (identical(parent, S7_object) || (is_class(parent) && parent@abstract)) { return(new_function( @@ -86,11 +86,16 @@ new_call <- function(call, args) { as.call(c(list(as.name(call)), args)) } -as_names <- function(x, named = FALSE) { +as_names <- function(x, named = FALSE, unnamed = "...") { if (named) { names(x) <- x } - lapply(x, as.name) + out <- lapply(x, as.name) + if (!is.null(nms <- names(out)) && length(unnamed)) { + nms[nms %in% unnamed] <- "" + names(out) <- nms + } + out } `append<-` <- function(x, after = length(x), value) { From 99f3d6bac741c34356a671c14fef29b32d0b32ec Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 27 Sep 2024 09:56:11 -0400 Subject: [PATCH 06/19] Revert "Pass along `...` unnamed in the constructor" This reverts commit 1a8f608801721efb028658069bc4eb0033e4c090. --- R/constructor.R | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/R/constructor.R b/R/constructor.R index 6f19f9fd..0594f595 100644 --- a/R/constructor.R +++ b/R/constructor.R @@ -1,7 +1,7 @@ new_constructor <- function(parent, properties) { properties <- as_properties(properties) arg_info <- constructor_args(parent, properties) - self_args <- as_names(names(arg_info$self), named = TRUE, unnamed = "...") + self_args <- as_names(names(arg_info$self), named = TRUE) if (identical(parent, S7_object) || (is_class(parent) && parent@abstract)) { return(new_function( @@ -86,16 +86,11 @@ new_call <- function(call, args) { as.call(c(list(as.name(call)), args)) } -as_names <- function(x, named = FALSE, unnamed = "...") { +as_names <- function(x, named = FALSE) { if (named) { names(x) <- x } - out <- lapply(x, as.name) - if (!is.null(nms <- names(out)) && length(unnamed)) { - nms[nms %in% unnamed] <- "" - names(out) <- nms - } - out + lapply(x, as.name) } `append<-` <- function(x, after = length(x), value) { From 56d9ac025b36d06a04e056e8113d6a0d17648166 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 27 Sep 2024 09:56:15 -0400 Subject: [PATCH 07/19] Revert "Always add `...` to end of formals." This reverts commit d02e6007a1dcb3b9738d6081f0f9792593f746e8. --- R/constructor.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/constructor.R b/R/constructor.R index 0594f595..38bab7e9 100644 --- a/R/constructor.R +++ b/R/constructor.R @@ -67,7 +67,7 @@ constructor_args <- function(parent, properties = list()) { is_dots <- vlapply(self_args, identical, quote(...)) if (any(is_dots)) { self_args[is_dots] <- NULL - append(self_args) <- alist(... = ) + append(self_args, after = which.max(is_dots)-1) <- alist(... = ) } list(parent = parent_args, From 0122a83c88bd3f1f56eb7c979989e387b40ae175 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 27 Sep 2024 09:56:17 -0400 Subject: [PATCH 08/19] Revert "Accept `...` as a default" This reverts commit 2629d4aa876fcf452da2bbb5b8c1522e7908592d. --- R/constructor.R | 10 ---------- R/property.R | 25 ++++++++----------------- man/new_property.Rd | 25 ++++++++----------------- 3 files changed, 16 insertions(+), 44 deletions(-) diff --git a/R/constructor.R b/R/constructor.R index 38bab7e9..d7572bbd 100644 --- a/R/constructor.R +++ b/R/constructor.R @@ -64,12 +64,6 @@ constructor_args <- function(parent, properties = list()) { function(name) prop_default(properties[[name]])) ) - is_dots <- vlapply(self_args, identical, quote(...)) - if (any(is_dots)) { - self_args[is_dots] <- NULL - append(self_args, after = which.max(is_dots)-1) <- alist(... = ) - } - list(parent = parent_args, self = self_args) } @@ -92,7 +86,3 @@ as_names <- function(x, named = FALSE) { } lapply(x, as.name) } - -`append<-` <- function(x, after = length(x), value) { - append(x, value, after) -} diff --git a/R/property.R b/R/property.R index a2c1e185..75c78eef 100644 --- a/R/property.R +++ b/R/property.R @@ -33,11 +33,9 @@ #' your code can assume that `value` has known type. #' @param default When an object is created and the property is not supplied, #' what should it default to? If `NULL`, it defaults to the "empty" instance -#' of `class`. Quoted calls become standard function argument promises in the -#' default constructor, evaluated at the time the object is constructed. A -#' value of `quote(...)` indicates that the property is not a named argument -#' in the constructor, and it is not set unless explicitly supplied. The -#' default value for the unset property in that case is `NULL`. +#' of `class`. This can also be a quoted call, which then becomes a standard +#' function promise in the default constructor, evaluated at the time the +#' object is constructed. #' @param name Property name, primarily used for error messages. Generally #' don't need to set this here, as it's more convenient to supply as a #' the element name when defining a list of properties. If both `name` @@ -73,12 +71,7 @@ #' args(clock) #' #' # These can be useful if you want to deprecate a property -#' # For example, say, at first you define -#' Person <- new_class("Person", properties = list( -#' firstName = class_character -#' )) -#' # Then, to deprecate `firstName` and rename it to `first_name` -#' Person <- new_class("Person", properties = list( +#' person <- new_class("person", properties = list( #' first_name = class_character, #' firstName = new_property( #' getter = function(self) { @@ -89,14 +82,12 @@ #' warning("@firstName is deprecated; please use @first_name instead", call. = FALSE) #' self@first_name <- value #' self -#' }, -#' default = quote(...) +#' } #' ) #' )) -#' hadley <- Person(firstName = "Hadley") # warning -#' hadley <- Person(first_name = "Hadley") -#' hadley@firstName # warning -#' hadley@firstName <- "John" # warning +#' hadley <- person(first_name = "Hadley") +#' hadley@firstName +#' hadley@firstName <- "John" #' hadley@first_name #' #' # Properties can have default values that are quoted calls. diff --git a/man/new_property.Rd b/man/new_property.Rd index 39f40e09..461e64b0 100644 --- a/man/new_property.Rd +++ b/man/new_property.Rd @@ -41,11 +41,9 @@ your code can assume that \code{value} has known type.} \item{default}{When an object is created and the property is not supplied, what should it default to? If \code{NULL}, it defaults to the "empty" instance -of \code{class}. Quoted calls become standard function argument promises in the -default constructor, evaluated at the time the object is constructed. A -value of \code{quote(...)} indicates that the property is not a named argument -in the constructor, and it is not set unless explicitly supplied. The -default value for the unset property in that case is \code{NULL}.} +of \code{class}. This can also be a quoted call, which then becomes a standard +function promise in the default constructor, evaluated at the time the +object is constructed.} \item{name}{Property name, primarily used for error messages. Generally don't need to set this here, as it's more convenient to supply as a @@ -94,12 +92,7 @@ try(clock(now = 10)) args(clock) # These can be useful if you want to deprecate a property -# For example, say, at first you define -Person <- new_class("Person", properties = list( - firstName = class_character -)) -# Then, to deprecate `firstName` and rename it to `first_name` -Person <- new_class("Person", properties = list( +person <- new_class("person", properties = list( first_name = class_character, firstName = new_property( getter = function(self) { @@ -110,14 +103,12 @@ Person <- new_class("Person", properties = list( warning("@firstName is deprecated; please use @first_name instead", call. = FALSE) self@first_name <- value self - }, - default = quote(...) + } ) )) -hadley <- Person(firstName = "Hadley") # warning -hadley <- Person(first_name = "Hadley") -hadley@firstName # warning -hadley@firstName <- "John" # warning +hadley <- person(first_name = "Hadley") +hadley@firstName +hadley@firstName <- "John" hadley@first_name # Properties can have default values that are quoted calls. From 33fe3c874b1a85fcaee1c28f1fa41c6e08aba0bf Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 27 Sep 2024 11:34:14 -0400 Subject: [PATCH 09/19] Update `new_property()` examples --- R/property.R | 27 +++++++++++++++++++++------ man/new_property.Rd | 27 +++++++++++++++++++++------ 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/R/property.R b/R/property.R index 75c78eef..9b0f4efc 100644 --- a/R/property.R +++ b/R/property.R @@ -79,15 +79,19 @@ #' self@first_name #' }, #' setter = function(self, value) { +#' if(is.null(value)) { +#' return(self) +#' } #' warning("@firstName is deprecated; please use @first_name instead", call. = FALSE) #' self@first_name <- value #' self #' } #' ) #' )) +#' hadley <- person(firstName = "Hadley") # Warning #' hadley <- person(first_name = "Hadley") -#' hadley@firstName -#' hadley@firstName <- "John" +#' hadley@firstName # Warning +#' hadley@firstName <- "John" # Warning #' hadley@first_name #' #' # Properties can have default values that are quoted calls. @@ -102,11 +106,22 @@ #' round(stopwatch()@totaltime) #' round(stopwatch(Sys.time() - 1)@totaltime) #' -#' # Properties can also have a 'missing' default value, making them -#' # required arguments to the default constructor. -#' # You can generate a missing arg with `quote(expr =)` or `rlang::missing_arg()` +#' # You can make a property required by the constructor either by: +#' # - relying on the validator to error with the default value, or by +#' # - setting the property default to a quoted error call. #' Person <- new_class("Person", properties = list( -#' name = new_property(class_character, default = quote(expr = )) +#' name = new_property(class_character, validator = function(value) { +#' if (length(value) != 1 || is.na(value) || value == "") +#' "must be a non-empty string" +#' })) +#' ) +#' try(Person()) +#' try(Person(1)) # class_character$validator() is also checked. +#' Person("Alice") +#' +#' Person <- new_class("Person", properties = list( +#' name = new_property(class_character, +#' default = quote(stop("@name is required"))) #' )) #' try(Person()) #' Person("Alice") diff --git a/man/new_property.Rd b/man/new_property.Rd index 461e64b0..f11e9bd5 100644 --- a/man/new_property.Rd +++ b/man/new_property.Rd @@ -100,15 +100,19 @@ person <- new_class("person", properties = list( self@first_name }, setter = function(self, value) { + if(is.null(value)) { + return(self) + } warning("@firstName is deprecated; please use @first_name instead", call. = FALSE) self@first_name <- value self } ) )) +hadley <- person(firstName = "Hadley") # Warning hadley <- person(first_name = "Hadley") -hadley@firstName -hadley@firstName <- "John" +hadley@firstName # Warning +hadley@firstName <- "John" # Warning hadley@first_name # Properties can have default values that are quoted calls. @@ -123,11 +127,22 @@ args(stopwatch) round(stopwatch()@totaltime) round(stopwatch(Sys.time() - 1)@totaltime) -# Properties can also have a 'missing' default value, making them -# required arguments to the default constructor. -# You can generate a missing arg with `quote(expr =)` or `rlang::missing_arg()` +# You can make a property required by the constructor either by: +# - relying on the validator to error with the default value, or by +# - setting the property default to a quoted error call. Person <- new_class("Person", properties = list( - name = new_property(class_character, default = quote(expr = )) + name = new_property(class_character, validator = function(value) { + if (length(value) != 1 || is.na(value) || value == "") + "must be a non-empty string" + })) +) +try(Person()) +try(Person(1)) # class_character$validator() is also checked. +Person("Alice") + +Person <- new_class("Person", properties = list( + name = new_property(class_character, + default = quote(stop("@name is required"))) )) try(Person()) Person("Alice") From 3a35cbbba6fb27474d295038489d7c37fd8b288a Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 27 Sep 2024 11:35:42 -0400 Subject: [PATCH 10/19] `new_object()`, set static props before dynamic props --- R/class.R | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/R/class.R b/R/class.R index fe6521fc..1e2bc6ab 100644 --- a/R/class.R +++ b/R/class.R @@ -253,16 +253,28 @@ new_object <- function(.parent, ...) { } args <- list(...) - nms <- names(args) + if ("" %in% names2(args)) { + stop("All arguments to `new_object(...)` must be named") + } + + dynamic_setter_prop_names <- names(Filter(\(p) is.function(p$setter), class@properties)) + + static_prop_vals <- args[setdiff(names(args), dynamic_setter_prop_names)] + dynamic_prop_vals <- args[intersect(names(args), dynamic_setter_prop_names)] + # TODO: Some type checking on `.parent`? object <- .parent - attr(object, "S7_class") <- class - class(object) <- class_dispatch(class) - # Set properties. This will potentially invoke custom property setters - for (name in names(args)) - prop(object, name, check = FALSE) <- args[[name]] + attrs <- c(list(class = class_dispatch(class), S7_class = class), + static_prop_vals, + attributes(object)) + attrs <- attrs[!duplicated(names(attrs))] + attributes(object) <- attrs + + # invoke custom property setters + for (name in names(dynamic_prop_vals)) + prop(object, name, check = FALSE) <- dynamic_prop_vals[[name]] # Don't need to validate if parent class already validated, # i.e. it's a non-abstract S7 class From 934703826c39c599e34dd0c89cf536505d23673d Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 27 Sep 2024 11:37:35 -0400 Subject: [PATCH 11/19] Better error call from default constructor --- R/constructor.R | 7 ++++++- tests/testthat/_snaps/constructor.md | 19 +++++++++++++++---- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/R/constructor.R b/R/constructor.R index d7572bbd..63df1e78 100644 --- a/R/constructor.R +++ b/R/constructor.R @@ -6,7 +6,12 @@ new_constructor <- function(parent, properties) { if (identical(parent, S7_object) || (is_class(parent) && parent@abstract)) { return(new_function( args = arg_info$self, - body = new_call("new_object", c(list(quote(S7_object())), self_args)), + body = as.call(c(quote(`{`), + # Force all promises here so that any errors are signaled from + # the constructor() call instead of the new_object() call. + unname(self_args), + new_call("new_object", c(list(quote(S7_object())), self_args)) + )), env = asNamespace("S7") )) } diff --git a/tests/testthat/_snaps/constructor.md b/tests/testthat/_snaps/constructor.md index 8c824bb9..a86c4857 100644 --- a/tests/testthat/_snaps/constructor.md +++ b/tests/testthat/_snaps/constructor.md @@ -4,13 +4,19 @@ new_constructor(S7_object, list()) Output function () - new_object(S7_object()) + { + new_object(S7_object()) + } Code new_constructor(S7_object, as_properties(list(x = class_numeric, y = class_numeric))) Output function (x = integer(0), y = integer(0)) - new_object(S7_object(), x = x, y = y) + { + x + y + new_object(S7_object(), x = x, y = y) + } Code foo <- new_class("foo", parent = class_character) @@ -51,13 +57,18 @@ new_constructor(foo1, list()) Output function () - new_object(S7_object()) + { + new_object(S7_object()) + } Code new_constructor(foo1, as_properties(list(y = class_double))) Output function (y = numeric(0)) - new_object(S7_object(), y = y) + { + y + new_object(S7_object(), y = y) + } # can use `...` in parent constructor From 2ba20343dd43c17baa33d2894c72722fbb630b42 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 27 Sep 2024 12:20:17 -0400 Subject: [PATCH 12/19] Disallow missing or `...` property default for now. --- R/property.R | 44 ++++++++++++++++++++++++++----- tests/testthat/test-constructor.R | 15 ++++++----- 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/R/property.R b/R/property.R index 9b0f4efc..3dcac86b 100644 --- a/R/property.R +++ b/R/property.R @@ -132,12 +132,7 @@ new_property <- function(class = class_any, default = NULL, name = NULL) { class <- as_class(class) - if (!is.null(default) && - !(is.call(default) || is.symbol(default)) && # allow promises - !class_inherits(default, class)) { - msg <- sprintf("`default` must be an instance of %s, not a %s", class_desc(class), obj_desc(default)) - stop(msg) - } + check_prop_default(default, class) if (!is.null(getter)) { check_function(getter, alist(self = )) @@ -162,6 +157,43 @@ new_property <- function(class = class_any, out } +check_prop_default <- function(default, class) { + if (is.null(default)) { + return() # always valid. + } + + if (is.call(default)) { + # A promise default; delay checking until constructor called. + return() + } + + if (is.symbol(default)) { + if (identical(default, quote(...))) { + # The meaning of a `...` prop default needs discussion + stop.parent("`default` cannot be `...`") + } + if (identical(default, quote(expr =))) { + # The meaning of a missing prop default needs discussion + stop.parent("`default` cannot be missing") + } + + # other symbols are treated as promises + return() + } + + if (class_inherits(default, class)) + return() + + msg <- sprintf("`default` must be an instance of %s, not a %s", + class_desc(class), obj_desc(default)) + + stop.parent(msg) +} + +stop.parent <- function(..., call = sys.call(-2)) { + stop(simpleError(.makeMessage(...), call)) +} + is_property <- function(x) inherits(x, "S7_property") #' @export diff --git a/tests/testthat/test-constructor.R b/tests/testthat/test-constructor.R index c51efd02..aa5eb75e 100644 --- a/tests/testthat/test-constructor.R +++ b/tests/testthat/test-constructor.R @@ -101,14 +101,15 @@ test_that("can create constructors with missing or lazy defaults", { Person <- new_class( name = "Person", properties = list( - # non-dynamic, default missing (required constructor arg) - first_name = new_property(class_character, default = quote(expr = )), + # non-dynamic, default error call (required constructor arg) + first_name = new_property(class_character, default = quote(stop( + 'argument "first_name" is missing, with no default'))), # non-dynamic, static default (optional constructor arg) middle_name = new_property(class_character, default = ""), - # non-dynamic, default missing (required constructor arg) (same as first_name) - last_name = new_property(class_missing | class_character), + # non-dynamic, nullable character + last_name = new_property(NULL | class_character), # non-dynamic, but defaults to the value of another property nick_name = new_property(class_character, default = quote(first_name)), @@ -133,15 +134,15 @@ test_that("can create constructors with missing or lazy defaults", { ) expect_equal(formals(Person), as.pairlist(alist( - first_name = , + first_name = stop('argument "first_name" is missing, with no default'), middle_name = "", - last_name = , + last_name = NULL, nick_name = first_name, birthdate = Sys.Date() ))) # no age expect_error(Person(), 'argument "first_name" is missing, with no default') - expect_error(Person("Alice"), 'argument "last_name" is missing, with no default') + expect_null(Person("Alice")@last_name) p <- Person("Alice", ,"Smith") From f795935f523a9d9160a943372d3439506e8c5ed3 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 27 Sep 2024 12:21:25 -0400 Subject: [PATCH 13/19] oldrel-4 compat: no `\()` --- R/class.R | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/R/class.R b/R/class.R index 1e2bc6ab..33d8d26a 100644 --- a/R/class.R +++ b/R/class.R @@ -257,7 +257,8 @@ new_object <- function(.parent, ...) { stop("All arguments to `new_object(...)` must be named") } - dynamic_setter_prop_names <- names(Filter(\(p) is.function(p$setter), class@properties)) + dynamic_setter_prop_names <- names(Filter(function(p) is.function(p$setter), + class@properties)) static_prop_vals <- args[setdiff(names(args), dynamic_setter_prop_names)] dynamic_prop_vals <- args[intersect(names(args), dynamic_setter_prop_names)] From 9fad909b4638701bcb6dcb390bbf90fa85284e66 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 27 Sep 2024 13:09:41 -0400 Subject: [PATCH 14/19] Add `read-only` example to `?new_property` --- R/property.R | 16 ++++++++++++++++ man/new_property.Rd | 16 ++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/R/property.R b/R/property.R index 3dcac86b..38e617da 100644 --- a/R/property.R +++ b/R/property.R @@ -125,6 +125,22 @@ #' )) #' try(Person()) #' Person("Alice") +#' +#' # You can mark a property as read-only after construction by +#' # providing a custom setter. +#' Person <- new_class("Person", properties = list( +#' birth_date = new_property( +#' class_Date, +#' setter = function(self, value) { +#' if(!is.null(self@birth_date)) { +#' stop("@birth_date is read-only", call. = FALSE) +#' } +#' self@birth_date <- as.Date(value) +#' self +#' } +#' ))) +#' person <- Person("1999-12-31") +#' try(person@birth_date <- "2000-01-01") new_property <- function(class = class_any, getter = NULL, setter = NULL, diff --git a/man/new_property.Rd b/man/new_property.Rd index f11e9bd5..c4709827 100644 --- a/man/new_property.Rd +++ b/man/new_property.Rd @@ -146,4 +146,20 @@ Person <- new_class("Person", properties = list( )) try(Person()) Person("Alice") + +# You can mark a property as read-only after construction by +# providing a custom setter. +Person <- new_class("Person", properties = list( + birth_date = new_property( + class_Date, + setter = function(self, value) { + if(!is.null(self@birth_date)) { + stop("@birth_date is read-only", call. = FALSE) + } + self@birth_date <- as.Date(value) + self + } +))) +person <- Person("1999-12-31") +try(person@birth_date <- "2000-01-01") } From 9cd508eebe41b541277326de76e9b06b13740460 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 27 Sep 2024 15:35:26 -0400 Subject: [PATCH 15/19] Update R/class.R Co-authored-by: Hadley Wickham --- R/class.R | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/R/class.R b/R/class.R index 33d8d26a..9591e66f 100644 --- a/R/class.R +++ b/R/class.R @@ -267,9 +267,11 @@ new_object <- function(.parent, ...) { # TODO: Some type checking on `.parent`? object <- .parent - attrs <- c(list(class = class_dispatch(class), S7_class = class), - static_prop_vals, - attributes(object)) + attrs <- c( + list(class = class_dispatch(class), S7_class = class), + static_prop_vals, + attributes(object) + ) attrs <- attrs[!duplicated(names(attrs))] attributes(object) <- attrs From 97d9ec38233fc5c6b3aff163f7db1ef304a9818d Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 27 Sep 2024 15:50:30 -0400 Subject: [PATCH 16/19] refactor `new_object()` --- R/class.R | 16 ++++++---------- R/property.R | 4 ++++ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/R/class.R b/R/class.R index 9591e66f..281b8e24 100644 --- a/R/class.R +++ b/R/class.R @@ -254,30 +254,26 @@ new_object <- function(.parent, ...) { args <- list(...) if ("" %in% names2(args)) { - stop("All arguments to `new_object(...)` must be named") + stop("All arguments to `...` must be named") } - dynamic_setter_prop_names <- names(Filter(function(p) is.function(p$setter), - class@properties)) - - static_prop_vals <- args[setdiff(names(args), dynamic_setter_prop_names)] - dynamic_prop_vals <- args[intersect(names(args), dynamic_setter_prop_names)] - + has_setter <- vlapply(class@properties[names(args)], prop_has_setter) # TODO: Some type checking on `.parent`? object <- .parent attrs <- c( list(class = class_dispatch(class), S7_class = class), - static_prop_vals, + args[!has_setter], attributes(object) ) attrs <- attrs[!duplicated(names(attrs))] attributes(object) <- attrs # invoke custom property setters - for (name in names(dynamic_prop_vals)) - prop(object, name, check = FALSE) <- dynamic_prop_vals[[name]] + prop_setter_vals <- args[has_setter] + for (name in names(prop_setter_vals)) + prop(object, name, check = FALSE) <- prop_setter_vals[[name]] # Don't need to validate if parent class already validated, # i.e. it's a non-abstract S7 class diff --git a/R/property.R b/R/property.R index 38e617da..8ed78baf 100644 --- a/R/property.R +++ b/R/property.R @@ -546,3 +546,7 @@ as_property <- function(x, name, i) { prop_is_read_only <- function(prop) { is.function(prop$getter) && !is.function(prop$setter) } + +prop_has_setter <- function(prop) is.function(prop$setter) + +prop_is_dynamic <- function(prop) is.function(prop$getter) From fd7c4f6facb374d6817dee12839cb11d94389a83 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 27 Sep 2024 15:56:07 -0400 Subject: [PATCH 17/19] add `error_call` to `check_prop_default()` --- R/property.R | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/R/property.R b/R/property.R index 8ed78baf..e4b405fc 100644 --- a/R/property.R +++ b/R/property.R @@ -173,7 +173,7 @@ new_property <- function(class = class_any, out } -check_prop_default <- function(default, class) { +check_prop_default <- function(default, class, error_call = sys.call(-1)) { if (is.null(default)) { return() # always valid. } @@ -186,11 +186,11 @@ check_prop_default <- function(default, class) { if (is.symbol(default)) { if (identical(default, quote(...))) { # The meaning of a `...` prop default needs discussion - stop.parent("`default` cannot be `...`") + stop(simpleError("`default` cannot be `...`", error_call)) } if (identical(default, quote(expr =))) { # The meaning of a missing prop default needs discussion - stop.parent("`default` cannot be missing") + stop(simpleError("`default` cannot be missing", error_call)) } # other symbols are treated as promises @@ -203,7 +203,7 @@ check_prop_default <- function(default, class) { msg <- sprintf("`default` must be an instance of %s, not a %s", class_desc(class), obj_desc(default)) - stop.parent(msg) + stop(simpleError(msg, error_call)) } stop.parent <- function(..., call = sys.call(-2)) { From a709d0bd520be84917ddcd36b1d3141c1e9027fa Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 27 Sep 2024 16:45:29 -0400 Subject: [PATCH 18/19] Add extended `new_property()` examples to vignette. --- R/property.R | 75 +-------------------- man/new_property.Rd | 75 +-------------------- vignettes/classes-objects.Rmd | 119 +++++++++++++++++++++++++++++++++- 3 files changed, 124 insertions(+), 145 deletions(-) diff --git a/R/property.R b/R/property.R index e4b405fc..70e53d9d 100644 --- a/R/property.R +++ b/R/property.R @@ -10,6 +10,9 @@ #' behaviour when modified. Dynamic properties are not included as an argument #' to the default class constructor. #' +#' See the "Properties: Common Patterns" section in `vignette("class-objects")` +#' for more examples. +#' #' @param class Class that the property must be an instance of. #' See [as_class()] for details. #' @param getter An optional function used to get the value. The function @@ -69,78 +72,6 @@ #' # argument to the default constructor #' try(clock(now = 10)) #' args(clock) -#' -#' # These can be useful if you want to deprecate a property -#' person <- new_class("person", properties = list( -#' first_name = class_character, -#' firstName = new_property( -#' getter = function(self) { -#' warning("@firstName is deprecated; please use @first_name instead", call. = FALSE) -#' self@first_name -#' }, -#' setter = function(self, value) { -#' if(is.null(value)) { -#' return(self) -#' } -#' warning("@firstName is deprecated; please use @first_name instead", call. = FALSE) -#' self@first_name <- value -#' self -#' } -#' ) -#' )) -#' hadley <- person(firstName = "Hadley") # Warning -#' hadley <- person(first_name = "Hadley") -#' hadley@firstName # Warning -#' hadley@firstName <- "John" # Warning -#' hadley@first_name -#' -#' # Properties can have default values that are quoted calls. -#' # These become standard function promises in the default constructor, -#' # evaluated at the time the object is constructed. -#' stopwatch <- new_class("stopwatch", properties = list( -#' starttime = new_property(class = class_POSIXct, default = quote(Sys.time())), -#' totaltime = new_property(getter = function(self) -#' difftime(Sys.time(), self@starttime, units = "secs")) -#' )) -#' args(stopwatch) -#' round(stopwatch()@totaltime) -#' round(stopwatch(Sys.time() - 1)@totaltime) -#' -#' # You can make a property required by the constructor either by: -#' # - relying on the validator to error with the default value, or by -#' # - setting the property default to a quoted error call. -#' Person <- new_class("Person", properties = list( -#' name = new_property(class_character, validator = function(value) { -#' if (length(value) != 1 || is.na(value) || value == "") -#' "must be a non-empty string" -#' })) -#' ) -#' try(Person()) -#' try(Person(1)) # class_character$validator() is also checked. -#' Person("Alice") -#' -#' Person <- new_class("Person", properties = list( -#' name = new_property(class_character, -#' default = quote(stop("@name is required"))) -#' )) -#' try(Person()) -#' Person("Alice") -#' -#' # You can mark a property as read-only after construction by -#' # providing a custom setter. -#' Person <- new_class("Person", properties = list( -#' birth_date = new_property( -#' class_Date, -#' setter = function(self, value) { -#' if(!is.null(self@birth_date)) { -#' stop("@birth_date is read-only", call. = FALSE) -#' } -#' self@birth_date <- as.Date(value) -#' self -#' } -#' ))) -#' person <- Person("1999-12-31") -#' try(person@birth_date <- "2000-01-01") new_property <- function(class = class_any, getter = NULL, setter = NULL, diff --git a/man/new_property.Rd b/man/new_property.Rd index c4709827..39f5b416 100644 --- a/man/new_property.Rd +++ b/man/new_property.Rd @@ -62,6 +62,9 @@ By specifying a \code{getter} and/or \code{setter}, you can make the property "dynamic" so that it's computed when accessed or has some non-standard behaviour when modified. Dynamic properties are not included as an argument to the default class constructor. + +See the "Properties: Common Patterns" section in \code{vignette("class-objects")} +for more examples. } \examples{ # Simple properties store data inside an object @@ -90,76 +93,4 @@ try(my_clock@now <- 10) # argument to the default constructor try(clock(now = 10)) args(clock) - -# These can be useful if you want to deprecate a property -person <- new_class("person", properties = list( - first_name = class_character, - firstName = new_property( - getter = function(self) { - warning("@firstName is deprecated; please use @first_name instead", call. = FALSE) - self@first_name - }, - setter = function(self, value) { - if(is.null(value)) { - return(self) - } - warning("@firstName is deprecated; please use @first_name instead", call. = FALSE) - self@first_name <- value - self - } - ) -)) -hadley <- person(firstName = "Hadley") # Warning -hadley <- person(first_name = "Hadley") -hadley@firstName # Warning -hadley@firstName <- "John" # Warning -hadley@first_name - -# Properties can have default values that are quoted calls. -# These become standard function promises in the default constructor, -# evaluated at the time the object is constructed. -stopwatch <- new_class("stopwatch", properties = list( - starttime = new_property(class = class_POSIXct, default = quote(Sys.time())), - totaltime = new_property(getter = function(self) - difftime(Sys.time(), self@starttime, units = "secs")) -)) -args(stopwatch) -round(stopwatch()@totaltime) -round(stopwatch(Sys.time() - 1)@totaltime) - -# You can make a property required by the constructor either by: -# - relying on the validator to error with the default value, or by -# - setting the property default to a quoted error call. -Person <- new_class("Person", properties = list( - name = new_property(class_character, validator = function(value) { - if (length(value) != 1 || is.na(value) || value == "") - "must be a non-empty string" - })) -) -try(Person()) -try(Person(1)) # class_character$validator() is also checked. -Person("Alice") - -Person <- new_class("Person", properties = list( - name = new_property(class_character, - default = quote(stop("@name is required"))) -)) -try(Person()) -Person("Alice") - -# You can mark a property as read-only after construction by -# providing a custom setter. -Person <- new_class("Person", properties = list( - birth_date = new_property( - class_Date, - setter = function(self, value) { - if(!is.null(self@birth_date)) { - stop("@birth_date is read-only", call. = FALSE) - } - self@birth_date <- as.Date(value) - self - } -))) -person <- Person("1999-12-31") -try(person@birth_date <- "2000-01-01") } diff --git a/vignettes/classes-objects.Rmd b/vignettes/classes-objects.Rmd index 861e80e8..cf13d467 100644 --- a/vignettes/classes-objects.Rmd +++ b/vignettes/classes-objects.Rmd @@ -194,6 +194,24 @@ empty <- new_class("empty", empty() ``` +A quoted call becomes a standard function promise in the default constructor, +evaluated at the time the object is constructed. +```{r} +stopwatch <- new_class("stopwatch", properties = list( + start_time = new_property( + class = class_POSIXct, + default = quote(Sys.time()) + ), + elapsed = new_property( + getter = function(self) { + difftime(Sys.time(), self@start_time, units = "secs") + } + ) +)) +args(stopwatch) +round(stopwatch()@elapsed) +round(stopwatch(Sys.time() - 1)@elapsed) +``` ### Computed properties It's sometimes useful to have a property that is computed on demand. @@ -224,6 +242,9 @@ x@length <- 20 ### Dynamic properties You can make a computed property fully dynamic so that it can be read and written by also supplying a `setter`. + +A `setter` is a function with arguments `self` and `value` that returns a modified object. + For example, we could extend the previous example to allow the `@length` to be set, by modifying the `@end` of the vector: ```{r} @@ -249,7 +270,103 @@ x@length <- 5 x ``` -A `setter` is a function with arguments `self` and `value` that returns a modified object. +### Common Patterns + +`getter`, `setter`, `default`, and `validator` can be used to implement many common patterns of properties. + +#### Deprecated properties + +A `setter` + `getter` can be used to to deprecate a property: + +```{r} +Person <- new_class("Person", properties = list( + first_name = class_character, + firstName = new_property( + getter = function(self) { + warning("@firstName is deprecated; please use @first_name instead", call. = FALSE) + self@first_name + }, + setter = function(self, value) { + if(is.null(value)) { + return(self) + } + warning("@firstName is deprecated; please use @first_name instead", call. = FALSE) + self@first_name <- value + self + } + ) +)) + +hadley <- Person(firstName = "Hadley") + +hadley <- Person(first_name = "Hadley") # no warning + +hadley@firstName + +hadley@firstName <- "John" + +hadley@first_name # no warning +``` + + +#### Required properties + +You can make a property required by the constructor either by: + +- relying on the validator to error with the default value, or by +- setting the property default to a quoted error call. + +```{r} +Person <- new_class("Person", properties = list( + name = new_property(class_character, validator = function(value) { + if (length(value) != 1 || is.na(value) || value == "") + "must be a non-empty string" + })) +) + +try(Person()) + +try(Person(1)) # class_character$validator() is also checked. + +Person("Alice") +``` + + +```{r} +Person <- new_class("Person", properties = list( + name = new_property(class_character, + default = quote(stop("@name is required"))) +)) + +try(Person()) + +Person("Alice") +``` + + +#### Frozen properties + +You can mark a property as read-only after construction by +providing a custom `setter`. + +```{r} +Person <- new_class("Person", properties = list( + birth_date = new_property( + class_Date, + setter = function(self, value) { + if(!is.null(self@birth_date)) { + stop("@birth_date is read-only", call. = FALSE) + } + self@birth_date <- as.Date(value) + self + } +))) + +person <- Person("1999-12-31") + +try(person@birth_date <- "2000-01-01") +``` + ## Constructors From d4c257bead02ae43ff12a878d8176e4da6fead94 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Mon, 30 Sep 2024 08:55:31 -0400 Subject: [PATCH 19/19] update "Deprecated Property" example --- vignettes/classes-objects.Rmd | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/vignettes/classes-objects.Rmd b/vignettes/classes-objects.Rmd index cf13d467..5c49026f 100644 --- a/vignettes/classes-objects.Rmd +++ b/vignettes/classes-objects.Rmd @@ -282,12 +282,14 @@ A `setter` + `getter` can be used to to deprecate a property: Person <- new_class("Person", properties = list( first_name = class_character, firstName = new_property( + class_character, + default = quote(first_name), getter = function(self) { warning("@firstName is deprecated; please use @first_name instead", call. = FALSE) self@first_name }, setter = function(self, value) { - if(is.null(value)) { + if (identical(value, self@first_name)) { return(self) } warning("@firstName is deprecated; please use @first_name instead", call. = FALSE) @@ -297,7 +299,9 @@ Person <- new_class("Person", properties = list( ) )) -hadley <- Person(firstName = "Hadley") +args(Person) + +hadley <- Person(firstName = "Hadley") hadley <- Person(first_name = "Hadley") # no warning