-
Notifications
You must be signed in to change notification settings - Fork 36
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
Updates to default constructor #438
Changes from all commits
1d3a5f6
ab1baf7
97be220
20d740e
208fb4e
434fa52
b9cbad0
7ee32dd
d45f394
eac36e4
ad3b0b2
159de6f
c136f3d
1ea6ebe
257eeb9
4c0c6ed
ec1b022
d717400
e7d792f
d069a8f
562af65
095e93a
7b8982f
9fcf3d7
83ab2cb
f381fca
f264c39
65c55d7
299ae86
9cfbd6e
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 |
---|---|---|
|
@@ -15,3 +15,6 @@ | |
^pkgdown$ | ||
^cran-comments\.md$ | ||
^CRAN-SUBMISSION$ | ||
^compile_commands\.json$ | ||
^\.cache$ | ||
^\.vscode$ |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,3 +3,6 @@ script.R | |
inst/doc | ||
docs | ||
.Rhistory | ||
compile_commands.json | ||
.cache | ||
.vscode |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
|
||
|
||
`%||%` <- function(x, y) if (is.null(x)) y else x | ||
|
||
new_function <- function(args = NULL, | ||
body = NULL, | ||
env = asNamespace("S7")) { | ||
as.function.default(c(args, body) %||% list(NULL), env) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -260,17 +260,9 @@ new_object <- function(.parent, ...) { | |
attr(object, "S7_class") <- class | ||
class(object) <- class_dispatch(class) | ||
|
||
supplied_props <- nms[!vlapply(args, is_class_missing)] | ||
for (prop in supplied_props) { | ||
prop(object, prop, check = FALSE) <- args[[prop]] | ||
} | ||
|
||
# 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)))) | ||
for (prop in missing_props) { | ||
prop(object, prop, check = FALSE) <- prop_default(class@properties[[prop]]) | ||
} | ||
# Set properties. This will potentially invoke custom property setters | ||
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 is a lot simpler 😄 |
||
for (name in names(args)) | ||
prop(object, name, check = FALSE) <- args[[name]] | ||
|
||
# Don't need to validate if parent class already validated, | ||
# i.e. it's a non-abstract S7 class | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,8 @@ | |
#' | ||
#' By specifying a `getter` and/or `setter`, you can make the property | ||
#' "dynamic" so that it's computed when accessed or has some non-standard | ||
#' behaviour when modified. | ||
#' behaviour when modified. Dynamic properties are not included as an argument | ||
#' to the default class constructor. | ||
#' | ||
#' @param class Class that the property must be an instance of. | ||
#' See [as_class()] for details. | ||
|
@@ -31,8 +32,10 @@ | |
#' The validator will be called after the `class` has been verified, so | ||
#' 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`, defaults to the "empty" instance | ||
#' of `class`. | ||
#' 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. | ||
#' @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` | ||
|
@@ -59,9 +62,14 @@ | |
#' my_clock <- clock() | ||
#' my_clock@now; Sys.sleep(1) | ||
#' my_clock@now | ||
#' # This property is read only | ||
#' # This property is read only, because there is a 'getter' but not a 'setter' | ||
#' try(my_clock@now <- 10) | ||
#' | ||
#' # Because the property is dynamic, it is not included as an | ||
#' # 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, | ||
|
@@ -81,14 +89,37 @@ | |
#' hadley@firstName | ||
#' hadley@firstName <- "John" | ||
#' 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. | ||
t-kalinowski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#' 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) | ||
#' | ||
#' # 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()` | ||
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. Should S7 provide 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.
|
||
#' Person <- new_class("Person", properties = list( | ||
#' name = new_property(class_character, default = quote(expr = )) | ||
#' )) | ||
#' try(Person()) | ||
#' Person("Alice") | ||
new_property <- function(class = class_any, | ||
getter = NULL, | ||
setter = NULL, | ||
validator = NULL, | ||
default = NULL, | ||
name = NULL) { | ||
class <- as_class(class) | ||
if (!is.null(default) && !class_inherits(default, 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) | ||
} | ||
|
@@ -131,7 +162,7 @@ str.S7_property <- function(object, ..., nest.lev = 0) { | |
} | ||
|
||
prop_default <- function(prop) { | ||
prop$default %||% class_construct(prop$class) | ||
prop$default %||% class_construct_expr(prop$class) | ||
} | ||
|
||
#' Get/set a property | ||
|
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.
Should this now be
quote(expression())
? (Not sure, as I haven't yet read the code that evaluates quoted defaults).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.
Or maybe they never get evaluated, they just get inlined into the functions arguments? That is potentially confusing because R won't display a argument with default
1:10
andquote(1:10)
differently. Or if it's a more complex object (like a data.frame), deparsing might be confusing? But maybe that just suggests that quoting the defaults should be the rule, not the exception?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.
Huh? not really .. (I assume you meant something 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.
I meant this problem:
Created on 2024-09-09 with reprex v2.1.0
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.
I think this might be what @hadley is describing (please confirm)
Created on 2024-09-09 with reprex v2.1.1
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.
I think you don't use
formals<-
properly -- you must usealist()
: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.
@mmaechler this is an issue with creating functions in non-standard ways, so we definitely want to use
list()
here.@t-kalinowski while inlining objects into the constructor args directly will generally be safe, I think it's going to potentially cause a lot of confusion.
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.
On my first pass at this (unpushed), I did have all branches in
base_default()
wrapped inquote(...)
. I reverted it once I realized it was unnecessary.Also, it's about 3x faster to call a constructor with an inlined value (I'm not sure how much this matters here; in absolute numbers, these are both very fast).
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.
I do think that this is more of a user facing documentation issue — i.e. I think we should generally recommend that the user wrap their default values in quote, unless they really know what they're doing.
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.
We can use NSE for
new_property(default)
and automatically quotedefault
. We could then allow usingI()
as an NSE escape hatch.That said, as I consider it, I'm coming to the conclusion that using NSE here isn't the best approach. The laziness of a default value should be explicit, clearly communicated to readers, and opt-in for authors.
I agree we'll want to update docs to discuss this and encourage usage of
quote()
.