Allow updating attributes in S7_data<-

[t-kalinowski]

closes #478

[hadley]

Maybe it should only modify attributes that aren't props? I'm not sure that matters in practice, but seems like the right principle?

[t-kalinowski]

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?

[hadley]

Or maybe using name or dim as a property should be an error?

[t-kalinowski]

Also, S7_data() completely removes the class attribute, which breaks some base S3 classes like factor, POSIXct, etc.

[hadley]

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.

[t-kalinowski]

Oh, I might be misunderstanding the role of S7_data() then. I was thinking of it as and un_S7() and re_S7()<-.

[t-kalinowski]

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.

[lawremi]

I had expected S7_data() to behave analogously to methods::S3Part(). It preserves attributes from the S3 "class". The methods package supports defining an S4 representation of an S3 class via setOldClass(S4class=).

Perhaps new_S3_class() could support an S7 class as its constructor= argument?

And/or preserve reserved S3 attributes like class, dim and names. The methods package never functioned properly when using slots with those names, so we always avoided them. I agree it would be good to explicitly disallow them unless we can figure out a way for them to work reliably.

[hadley]

IIRC S7_data() was supposed to be equivalent to @.Data:

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. print()).

I suspect we should all agree on what the point of S7_data() really is before we make any changes 😄

[hadley]

Oh but see also #380 where this behaviour confused past-Hadley.