box icon indicating copy to clipboard operation
box copied to clipboard

FAQ: why do I need to register methods for S3 generics from other modules/packages?

Open klmr opened this issue 3 years ago • 3 comments

klmr avatar Oct 19 '22 13:10 klmr

Context: Reddit discussion; to wit:

You’ve accurately described ‘box’ semantics here.

No, I accurately described how S3 methods are handled in R.

Right, the issue is that, as you wrote previously (emphasis mine):

You don't need to register S3 methods if the context is local. I.e., if the S3 method is defined in the same environment as it is used.

With modules, S3 methods are not generally defined in the same environment in which they are used (when they are, there’s no issue as you noted). The problem occurs if a user wants to define an S3 method inside a module and use it outside that module. In this case, R won’t find the appropriate method because it only searches the environment in which a generic was called, not the one in which the generic was defined (unless the S3 method is explicitly registered — and, to reiterate ‘box’ does this automatically behind the scenes for the user). — I assume you know this, since you added the relevant code to ‘import’ if I understood you correctly.

klmr avatar Aug 27 '23 10:08 klmr

I wonder if this is related to this: https://github.com/ianmcook/implyr/issues/57

tyner avatar Oct 20 '23 19:10 tyner

I don’t think it’s related, since S3 classes in packages should just work with ‘box’. And the issue you’re linking to is related to S4 at any rate, not S3.

S4 is another whole can of worms but you should be able to load and use packages that define S4 classes with ‘box’ without issues. Without being an S4 expert, it seems to me that the ’implyr’ package does some questionable things with regards to S4 registration which I believe will cause issues. In particular, it seems to explicitly register S4 classes in the global environment rather than in the package namespace. I don’t think packages are supposed to do that.

I’d love to help diagnose that issue but I don’t have an Impala database handy, and setting up a mock database doesn’t seem trivial without preexisting knowledge. One thing you could try (to see whether this is related to ‘box’) is to perform a manual import of the relevant functions; that is, instead of using box::use(), use the following code:

src_impala <- implyr::src_impala
dbGetQuery <- DBI::dbGetQuery

options(warn=1)
impala <- src_impala(drv = odbc::odbc(),
                     dsn = "my_dsn",
                     database = "default",
                     bigint = "character",
                     auto_disconnect = FALSE
                     )

ret <- dbGetQuery(impala, "show databases")

If this also fails, the error is in ‘implyr’.

klmr avatar Oct 20 '23 20:10 klmr