klmr / box

Write reusable, composable and modular R code
https://klmr.me/box/
MIT License
829 stars 47 forks source link

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

Open klmr opened 1 year ago

klmr commented 10 months ago

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.

tyner commented 8 months ago

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

klmr commented 8 months ago

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’.