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

How to define S3 / S7 methods on namespaced classes #333

Closed
jonclayden opened this issue Sep 7, 2023 · 7 comments · Fixed by #334
Closed

How to define S3 / S7 methods on namespaced classes #333

jonclayden opened this issue Sep 7, 2023 · 7 comments · Fixed by #334

Comments

@jonclayden
Copy link

I'm kicking the tyres on the S7 project by trying to create a fork of one of my packages that uses it (instead of reference classes). One thing I've got stuck on so far is defining methods for package-namespaced classes.

The documentation recommends specifying the package, viz.

library(S7)
myarray <- new_class("myarray", package="mypackage", properties=list(data=class_atomic, dims=class_integer))

But now, when I try to define a simple S3 method like

method(dim, myarray) <- function (x) x@dims

it doesn't seem to be picked up:

library(mypackage)
a <- myarray(data=1:4, dims=c(2L,2L))
dim(a)
## NULL

I am calling S7::methods_register() in .onLoad() and exporting the myarray constructor.

The issue appears to be that the class of a is now mypackage::myarray, which doesn't match the method so it isn't used. But scoping the class in the call to method<- doesn't work either. If I remove the package="mypackage" bit then everything works.

I'm not sure if I've missed the correct incantation from the documentation, or this a bug, but any pointers would be appreciated. Thanks!

@mmaechler
Copy link
Collaborator

The issue appears to be that the class of a is now mypackage::myarray .....

That really should not happen. As you have attached mypackage (by library(..)), myarray is visible.
.. and of course mypackage::myarray gets to the same object.
However the name of the class should always by "myarray" and it should have a property saying where it comes from, i.e. from namespace "mypackage" but that should really not be part of the class name.

BTW, I think we have a missing feature in S7 also: I did not find (in S7) utility functions listing
generics, methods, classes in a package {as we have in the methods package for S4 classes, generics, methods}.

Back to your problem "doctor, it hurts if I do this ..... then, don't do that!"
Inside package code, I'd find it very strange to have to mention the package itself at all,
when defining classes, generics, or methods.
The R code should not have to say that they are from package <foo>; they are from that package/namespace and exported (or not), etc... but the defining R code should not have to mention the package anywhere I think.

@mmaechler mmaechler changed the title How to define S3 methods on namespaced classes How to define S7 methods on namespaced classes Sep 8, 2023
@mmaechler mmaechler changed the title How to define S7 methods on namespaced classes How to define S3 / S7 methods on namespaced classes Sep 8, 2023
@jonclayden
Copy link
Author

The issue appears to be that the class of a is now mypackage::myarray .....

That really should not happen. As you have attached mypackage (by library(..)), myarray is visible. .. and of course mypackage::myarray gets to the same object. However the name of the class should always by "myarray" and it should have a property saying where it comes from, i.e. from namespace "mypackage" but that should really not be part of the class name.

Agreed.

BTW, I think we have a missing feature in S7 also: I did not find (in S7) utility functions listing generics, methods, classes in a package {as we have in the methods package for S4 classes, generics, methods}.

Yes, this would certainly be helpful.

Back to your problem "doctor, it hurts if I do this ..... then, don't do that!" Inside package code, I'd find it very strange to have to mention the package itself at all, when defining classes, generics, or methods. The R code should not have to say that they are from package <foo>; they are from that package/namespace and exported (or not), etc... but the defining R code should not have to mention the package anywhere I think.

That would simplify the client package story a bit, if the back-end can still keep track of everything.

@hadley
Copy link
Member

hadley commented Sep 8, 2023

@jonclayden do you have a simple package reprex somewhere?

@jonclayden
Copy link
Author

Now created:

remotes::install_github("jonclayden/S7issue333")

library(S7issue333)
a <- myarray(data=1:4, dims=c(2L,2L))
dim(a)
## NULL
class(a)
## [1] "S7issue333::myarray" "S7_object" 

@hadley
Copy link
Member

hadley commented Sep 8, 2023

@mmaechler we currently have an explicit class argument because it's useful for testing and sometimes you need to move classes from one package to another while preserving the original package name. I'm not sure we ever discussed how to do that automatically.

I'm pretty sure we did discuss that the easiest approach for package namespace using S3 classes was to include the package name in the class name; anything else would break S7/S3 compatibility in a way that we didn't want.

@hadley
Copy link
Member

hadley commented Sep 8, 2023

Hmmm, looks like you don't even need to be in a package for this to be a problem:

library(S7)

myarray <- new_class("myarray", package = "foo")
method(dim, myarray) <- function (x) 1
dim(myarray())
#> NULL

Created on 2023-09-08 with reprex v2.0.2

@jonclayden
Copy link
Author

No, indeed – the example in the first comment runs as-is, if you drop the library(mypackage) line.

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 a pull request may close this issue.

3 participants