Allow updating attributes in S7_data<-
#479
Conversation
| @@ -29,7 +29,7 @@ S7_data <- function(object) { | |||
| `S7_data<-` <- function(object, check = TRUE, value) { | |||
| attrs <- attributes(object) | |||
| object <- value | |||
| attributes(object) <- attrs | |||
| attributes(object) <- modify_list(attrs, attributes(value)) | |||
There was a problem hiding this comment.
Maybe it should only modify attributes that aren't props? I'm not sure that matters in practice, but seems like the right principle?
There was a problem hiding this comment.
As I think about this, I see it's trickier than I first thought. What if a user declares a base attribute as a property? For example:
MyList := new_class(class_list, properties = list(
names = class_character
))
MyArray := new_class(class_double, properties = list(
dim = class_integer
))Right now, S7_data() drops names and dim, and `S7_data<-`() doesn't update them, neither of which is what a user would likely want.
Maybe we should encourage more explicit usage of unclass() and `attributes<-`() to "temporarily" retrieve and update the objects?
There was a problem hiding this comment.
Or maybe using name or dim as a property should be an error?
There was a problem hiding this comment.
Also, S7_data() completely removes the class attribute, which breaks some base S3 classes like factor, POSIXct, etc.
There was a problem hiding this comment.
I think that's to be expected? The goal of S7_data is to give you the underlying base object. OTOH if you're extending an S3 class, it seems like there will be case where you want to get the underlying S3 object.
There was a problem hiding this comment.
Oh, I might be misunderstanding the role of S7_data() then. I was thinking of it as and un_S7() and re_S7()<-.
There was a problem hiding this comment.
I reached for it because in ?super we suggest S7_data() as the way to achieve the equivalent of calling NextMethod() in an S3 generic method. Since S7_data() drops the class attribute, we may want to update ?super docs.
|
I had expected Perhaps And/or preserve reserved S3 attributes like |
|
IIRC A <- setClass("A", contains = "character")
a <- A("abc")
a@.Data
#> [1] "abc"
library(S7)
A <- new_class("A", parent = class_character)
a <- A("abc")
S7_data(a)
#> [1] "abc"Created on 2024-10-28 with reprex v2.1.0 You need this sometimes in order to pass to base generics that don't have a way to ignore attributes (e.g. I suspect we should all agree on what the point of |
|
Oh but see also #380 where this behaviour confused past-Hadley. |
mmaechler
changed the title
S7_data<-S7_data<-
closes #478