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

Automatic delayed S3 method registrations for R 3.6.0? #796

Closed
talgalili opened this issue Sep 20, 2018 · 14 comments
Closed

Automatic delayed S3 method registrations for R 3.6.0? #796

talgalili opened this issue Sep 20, 2018 · 14 comments

Comments

@talgalili
Copy link

@talgalili talgalili commented Sep 20, 2018

I just got the following email from Kurt Hornik (regarding my dendextend package):

Dear maintainer,

Apparently your package has S3 methods it does not register via
NAMESPACE and instead exports, so that these methods can only be found
on the search path. The CRAN run time tests find at least

S3 method lookup found 'as.phylo.dendrogram' on search path 

there may be more.

Presumably, you did not register the methods as this requires the
generic to be imported, and hence the namespace of the generic and all
its dependencies to be loaded, which maintainers often prefer to avoid
for S3 methods which are really only needed when the namespace of their
generic is loaded.

For the current development version of R, we have recently added a
simple mechanism for delayed S3 registration: using

if(getRversion() >= "3.6.0") {
S3method(pkg::gen, cls)
}

will register 'gen.cls' as S3 method for class 'cls' and generic 'gen'
from package 'pkg' only when the namespace of 'pkg' is loaded.
(Similarly, one can use

if(getRversion() >= "3.6.0") {
S3method(pkg::gen, cls, fun)
}

in case the method is named 'fun' and not 'gen.cls'.)

Can you please add the necessary delayed S3 method registrations to your
NAMESPACE, and submit a new version of your package as quickly as
possible?

I wrote to him that I use @export to get the S3 method registered (see here: https://github.com/talgalili/dendextend/blob/c0885f64686a1a7ff86eb06dde8b12e1233549cd/R/ape.R#L24 ), and that I cannot manually edit NAMESPACE since it is managed automatically via roxygen2.

What should be a solution for this?

One option is that roxygen2 will move to using

 if(getRversion() >= "3.6.0") {
      S3method(pkg::gen, cls)
 }

Another is to allow some areas of the NAMESPACE to have manual parts, and the rest will be auto-generated (is this already possible now?!).
Any other thoughts?

Thanks.

@gaborcsardi
Copy link
Collaborator

@gaborcsardi gaborcsardi commented Sep 20, 2018

It would make sense to directly support delayed resgistration in Roxygen, I think.

For now, you can use @rawNamespace, see under https://github.com/klutometis/roxygen/blob/cc34200a598d02850020c874fdd0d6a82747f28e/NEWS.md#new-features-1

@talgalili

This comment has been hidden.

@talgalili

This comment has been hidden.

@gaborcsardi

This comment has been hidden.

@hadley
Copy link
Member

@hadley hadley commented Oct 23, 2018

This requires some thought because it automatically adds a R 3.6 dependency to your package, and there are existing ways to achieve the same result that work for R 3.1 and up.

@bastistician
Copy link
Contributor

@bastistician bastistician commented Oct 23, 2018

Just for the record: Non-registered but export()ed S3 methods for generics from non-imported packages (e.g., function as.phylo.dendrogram() in dendextend) are still found from calls of the corresponding generic in default (!) R 3.5.1 (and older) sessions and even in current R-devel. By "default" I mean environments without the special setting _R_S3_METHOD_LOOKUP_BASEENV_AFTER_GLOBALENV_=TRUE (which is used by some platforms of the CRAN check farm).

While some packages have long-adopted proper but manual ways of delayed S3 method registration in .onLoad(), I think many packages actually relied on the search of non-registered S3 methods. Those packages probably exported the function using roxygen's "export" tag (at least I did). Since I do not want my package to require R 3.6.0, my current solution in https://github.com/bastistician/polyCub/blob/v0.7.0/NAMESPACE is to simply keep that export declaration and add an S3method declaration conditional on getRversion() >= "3.6.0" (exactly as requested by CRAN).

So a possible new feature of roxygen could be a revived "S3method" tag, which, if specified in the form @S3method pkg::gen cls, inserts

if(getRversion() >= "3.6.0") {
S3method(pkg::gen, cls)
}

into the NAMESPACE -- omitting the if-clause only if the package DESCRIPTION states a dependency on R >= 3.6.0 (or higher).

@Enchufa2
Copy link

@Enchufa2 Enchufa2 commented Dec 4, 2018

I received the same email. It would be even better to generate something like the following:

if(getRversion() >= "3.6.0") {
  S3method(pkg::gen, cls)
} else {
  export(gen.cls)
}

@bastistician
Copy link
Contributor

@bastistician bastistician commented Dec 4, 2018

@Enchufa2, note that your suggestion would cause the package's API to depend on the R version in use, which I think is a bad idea. For backwards compatibility, I suggest package authors to keep such an export declaration for a while at least. It could be removed upon a major package update which then introduces a strict dependency on R >= 3.6.0.

@Enchufa2
Copy link

@Enchufa2 Enchufa2 commented Dec 4, 2018

My suggestion is backwards compatible, because it keeps such an export for R < 3.6.0, and it removes it for R >= 3.6.0, using the new mechanism instead. I don't see the problem.

@hadley
Copy link
Member

@hadley hadley commented Dec 4, 2018

@Enchufa2 Exporting a method in that way is not something that we have ever recommended, and so is unlikely to be something we would automate.

@tim-salabim
Copy link

@tim-salabim tim-salabim commented Dec 9, 2018

Here's a reply I received from Kurt Hornik regarding the issue:

In principle, it would be best to stop exporting methods when no longer
necessary. I.e., something like

if(getRversion() >= "3.6.0") {
S3method(pkg::gen, cls)
} else {
export(gen.cls)
}

Best
-k

which is what @Enchufa2 has in mind.

@bastistician
Copy link
Contributor

@bastistician bastistician commented Jan 22, 2019

I will try to clarify my previous comment. NAMESPACE exports define the package's API. The point is: with the above if-else-construct, the call package::gen.cls() will stop working if I update my R installation to R 3.6.0 even if I do not update the package to a newer version.

Following semantic versioning, removing a function from the package's API would actually require a major version update since that is a backwards-incompatible change. It is certainly possible that package users or authors have relied on a specific S3 method being accessible as package::gen.cls(), such as many S3 methods in base R, e.g., stats::update.formula.

For these reasons, I think the proper approach is to add delayed method registration for R >= 3.6.0 compliance and only remove such exports explicitly as part of a later package update (which then would also introduce a package dependence on R >= 3.6.0).

@hadley
Copy link
Member

@hadley hadley commented Aug 22, 2019

I think the most likely path that I'll take is something like @exportS3Method pkg::generic which would generate S3method(pkg::generic, class) (finding the class automatically from the function name). This would implicitly add an R 3.6 dependency; if you wanted to also work with older versions, you'd instead use vctrs::s3_register() or similar.

@bwiernik
Copy link

@bwiernik bwiernik commented Sep 5, 2021

Just noting for anyone searching for how to handle conditional exports with roxygen2 in the future:

As of roxygen2 6.1.0, you can export with the if (getRversion() >= "3.6.0") {S3method(pkg::fun, class)} structure for conditional exports using:

#' @rawNamespace if (getRversion() >= "3.6.0") {
#'   S3method(pkg::fun, class)
#'   S3method(pkg::fun, another_class)
#' }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants