importing S3 methods

[J-Moravec]

Not entirely related to the #16 so raising a new issue so we can discuss this problem.

Relevant posts from the old thread: https://github.com/rticulate/import/issues/16#issuecomment-920524702 https://github.com/rticulate/import/issues/16#issuecomment-920938522

Found a potential solution. The problem is that the S3 method needs to be registered. From UseMethod documentation (here on page 629):

Namespaces can register methods for generic functions. To support this, UseMethod and NextMethod search for methods in two places: in the environment in which the generic function is called, and in the registration data base for the environment in which the generic is defined (typi- cally a namespace). So methods for a generic function need to be available in the environment of the call to the generic, or they must be registered. (It does not matter whether they are visible in the environment in which the generic is defined.) As from R 3.5.0, the registration data base is searched after the top level environment (see topenv) of the calling environment (but before the parents of the top level environment).

If you look at packages with S3, you will see in NAMESPACE:

S3method(generic, class)

S3method is not available, but there is a related function with a signature: .S3method(generic, class, function).

As a local hotfix, modifying the module_catter.R with:

# module_catter.R

catter <- function(x) {
  class(x) <- "catter"
  x
}

print.catter <- function(x) {
  cat(x)
  invisible(x)
}

.S3method("print", "catter", print.catter) # It looks like methods are not limited to generic.class calls

and then running:

import::from(module_catter.R, catter)
x <- catter(letters[1:3]) # Construct catter
x                                  # Uses print.catter()
#> a b c

As a hotfix, adding .S3method call after every definition of S3 method will fix the issue.

Modifying import to look at function names and constructing individual calls of .S3method for function names with x.y pattern should do the trick. Or maybe use the registerS3method which is the internal workhorse of the .S3method, if importing to a particular environment is required, instead of the default parent.frame()

Alternatively, just add to documentation that registering S3 methods is required.

[torfason]

Nice find, there are many things to learn about the intricacies of R environments and dispatching. :-)

I looked at this and created a small proof-of-concept package for it, see the package and examples in the readme for the package at:

https://github.com/torfason/catterpoc

My conclusion is that the difference is not between library()and import() but between whether a method is defined in a package or in a script.

In both cases, the R documentation suggests that in addition to naming a function the correct way (function.class), one need to explicitly declare that a function really is an S3 method rather than just a function that happens to have a period in its name.

I think the solution of clearly documenting this approach, that modules for use with import should declare S3 methods using .S3method() is the best approach to this.

I'm not too fond of the idea of finding all methods containing a period and calling .S3method() behind the scenes. There are lots of non-S3 reasons for functions with periods and this could cause unexpected side effects. Even checking that such functions exist in pairs (class() and fun.class() both exist in the same modules) could also pick up false negatives, because the class() function may just be a regular function and not a constructor. (Although Roxygen does seem to have some sort of heuristic, which might be an argument for it, but even there, an explicit @export statement is required for methods).

[torfason]

Looks like the Roxygen heuristic checks to see if a generic method has been defined. And it does make errors, for example if the package includes and exports t.test.catter_a() roxygen will add the following to NAMESPACE:

S3method(t,test.catter_a)

(instead of S3method(t.test,catter_a))

:-P

[J-Moravec]

Yes, roxygen's algorithm does not allow for function with the pattern x.y and assumes them to be S3 classes. This can be overruled, but R CMD check (at least the one run by devtools) still throws a warning, so I am not sure they are still supported and not just a part of legacy code, because the ?majority? of new packages instead use the x_y pattern.

https://github.com/bioDS/phyloRNA/commit/0fc8a95ea3687cd983870c4cd70f110aa351d222 (see the commit comment)

If I had a vote, I would go for an automatic export with a heuristic similar to roxygen as it mimics most closely the default behaviour when the S3 methods are defined locally (and are automatically dispatched). And allow for turning off and adding a note for manual registration. This feels to me as a most consistent behaviour. I am happy to write the required code and create a pull request with tests (would take some time).

This means that the following would have consistent behaviour:

Locally defined and used in a module:

``` # module_catter.r catter <- function(x) { class(x) <- "catter" x } print.catter <- function(x) { cat(x) invisible(x) } cattering <- function(){ x = catter("And thats all, folks!") print(x) } ``` ``` # catter.r import::from("module_catter.r", "cattering") cattering() ```

Defined in module and imported:

``` # module_catter.r catter <- function(x) { class(x) <- "catter" x } print.catter <- function(x) { cat(x) invisible(x) } # currently needs to be registered using S3method() ``` ``` # catter.r import::from("module_catter.r", "catter") x = catter("And thats all, folks!") print(x) ```

Defined in module and imported using import::here:

``` # module_catter.r catter <- function(x) { class(x) <- "catter" x } print.catter <- function(x) { cat(x) invisible(x) } ``` ``` # catter.r import::here("module_catter.r", "catter", "print.catter") x = catter("And thats all, folks!") print(x) # works because here import print.catter to a local environment ```

Right now, only the first and third option works.

[torfason]

I think the .S3method() function is clearly the way to go in order to improve the way import handles methods. And a feature idea coupled with an implementation offer is worth a thousand without :-)

So I think if you are willing to contribute something on this that would be great, and merit a new release (which I've been pondering, but not gone forward with, partly because of general time constraints and partly because of a bothersome issue with the GitHub actions that seems to prevent commit checks from passing even if nothing related to the checks has changed).

With such a feature, I would like to to make sure that backwards compatibility is maintained, because import has enough users that regressions lead to dissatisfied customers.

Some thoughts:

Yes, roxygen's algorithm does not allow for function with the pattern x.y and assumes them to be S3 classes.

I updated the catterpoc package to check this out, and it seems that roxygen checks to see if a method is found. Exporting a function such as b.test.classname() as a regular method, but t.test.classname() gets exported as a method (as does print.classname() which is probably among the more common examples).

And allow for turning off and adding a note for manual registration.

In light of backwards compatibility, I would find an on-switch more attractive than an off-switch, but possibly with a message or a warning if it finds a method that it "would like to " export as an S3 method.

Perhaps an .S3=TRUE parameter in import::from() is attractive (with a default of .S3=FALSE).

For more extensive use, I've thought for a while that having the defaults be configureable might be attractive, but again that would need to happen without too much clutter – and require some documentation and tests.

create a pull request with tests (would take some time)

Sounds great, perhaps we should continue discussing here a bit before diving in, to make sure everyone is agreed on the approach. And it would be great if others are willing to chime in on this.

[torfason]

@J-Moravec, have you had time to look at this any more?

As the number of outstanding issues for a new release gets smaller, we probably want to make a decision on whether to (a) hold off on releasing for a couple more weeks in order to resolve this issue (#58) and #53 as well, or (b) do a 1.2.1 release now, with an eye on following it up with a 1.3.0 relatively soon with those added features.

[J-Moravec]

Sorry, didn't have time yet. Give me until the end of the week (including the weekend). But feel free to release bugfix 1.2.1 with S3 being the new feature of 1.3.0.

[J-Moravec]

Fixed in #65

[torfason]

Looks great, PR is now in dev and slated for 1.3.0 release.

[torfason]

All features/fixes targeted for v1.3.0 are now in dev and master has been forwarded to match dev, and the version bumped to correspond to that. All checks required for a CRAN release see to be passing, and the plan is to submit to CRAN after the weekend, unless any unknown issues are uncovered until then. In the meantime, it would be much appreciated if the people involved in the new features/fixes could check out this could install and try out the most recent version from GitHub, for example using:

pak::pak("rticulate/import")

(remotes::install_github("rticulate/import") or devtools::install_github("rticulate/import") should give the same result)

Thanks to everyone for their contributions!

[torfason]

Fixed in v1.3.0