Skip to content
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.

Sign up for GitHub

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

Allow updating attributes in S7_data<- #479

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Allow updating attributes in S7_data<- #479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
02c1e78
set updated attributes in `S7_data<-`
t-kalinowski Oct 25, 2024
3d4bfd7
add test
t-kalinowski Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion R/data.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@t-kalinowski t-kalinowski Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@t-kalinowski t-kalinowski Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (isTRUE(check)) {
validate(object)
}
Expand Down
29 changes: 29 additions & 0 deletions tests/testthat/test-data.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,32 @@ describe("S7_data", {
expect_equal(S7_data(x), "bar")
})
})


describe("S7_data<-", {

it("uses updated 'names'", {
# local_methods(`$<-`, `[[<-`) # no support for unregistering S3 generic methods

write_once_list <- new_class("write_once_list", class_list,
constructor = function(...) new_object(list(...))
)

method(`$<-`, write_once_list) <-
method(`[[<-`, write_once_list) <- function(x, name, value) {
.x <- S7_data(x)
stopifnot(is_string(name))
if (hasName(.x, name))
stop("entry exists: ", name)
.x[[name]] <- value
S7_data(x) <- .x
x
}
w <- write_once_list(x = 3, y = 4)
w$bar <- 1
expect_equal(names(w), c("x", "y", "bar"))
expect_error(w$bar <- 2, "entry exists:")

})

})
Loading