RConsortium / S7

S7: a new OO system for R
https://rconsortium.github.io/S7
Other
387 stars 33 forks source link

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

Closed jonclayden closed 1 year ago

jonclayden commented 1 year ago

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 commented 1 year ago

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.

jonclayden commented 1 year ago

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 commented 1 year ago

@jonclayden do you have a simple package reprex somewhere?

jonclayden commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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