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.

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

Use explicit class in S4_register() #214

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Use explicit class in S4_register() #214

wants to merge 21 commits into from

Conversation

hadley
Copy link
Member

@hadley hadley commented Mar 22, 2022

No description provided.

@hadley hadley marked this pull request as ready for review March 22, 2022 19:38
@hadley hadley requested review from DavisVaughan and lawremi March 23, 2022 17:49
@hadley
Copy link
Member Author

hadley commented Mar 23, 2022

@lawremi Does this look like what you expect? Are there other S4 usage scenarios that you think I should test?

R/S4.R Outdated Show resolved Hide resolved
R/S4.R Show resolved Hide resolved
R/S4.R Outdated Show resolved Hide resolved
R7_base = double_to_numeric(x$class),
R7_S3 = x$class[[1]],
R7_union = "ANY",
stop("Unsupported")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess you could have an R7 property of class_missing, and that would trigger this? That would be somewhat weird though. Everything else seems to be handled

Comment on lines 114 to 118
foo <- new_class("foo")
foo2 <- new_class("foo2", foo)

S4_register(foo)
S4_register(foo2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you get an error if you reverse the order of the S4_register() calls?

tests/testthat/test-S4.R Outdated Show resolved Hide resolved
@lawremi
Copy link
Collaborator

lawremi commented Apr 14, 2022

What about setting the S4 class prototype using the default values from the R7 properties?

I guess we don't support virtual classes in R7 yet, but if we did, we'd obviously want to translate that, as well.

@hadley
Copy link
Member Author

hadley commented Apr 14, 2022

Do you have an example of what prototype should look like? This is one of the arguments that the docs suggest you shouldn't use.

@lawremi
Copy link
Collaborator

lawremi commented Apr 15, 2022

The prototype can be constructed with prototype(), like prototype(slot=default_value).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants