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

Register S3 methods with @exportS3Method instead of onLoad() #457

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

olivroy
Copy link
Contributor

@olivroy olivroy commented May 14, 2024

  • redocument with CRAN roxygen2 and commit left-overs.

vec_ptype2.fs_bytes.double <- function(x, y, ...) {
x
}
#' @exportS3Method vctrs::vec_ptype2
vec_ptype2.double.fs_bytes <- function(x, y, ...) {
y
}

# Note order of class is the opposite as for ptype2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this comment is still relevant?

@gaborcsardi
Copy link
Member

Thanks! What are the pros and cons for this?

@olivroy
Copy link
Contributor Author

olivroy commented May 14, 2024

I am not sure. But apparently, since you depend on R 3.6, it can work. From roxygen 7.3.0 blog post, https://roxygen2.r-lib.org/dev/articles/namespace.html#s3

Typically, you will write methods for generics that are either defined in the current package or a package that is a hard dependency2 of your package. Sometimes, however, you will want to write a method for a suggested dependency. In this case, @export will not work because it assumes the generic is included or imported in your NAMESPACE. Instead, use @exportS3Method. This will use “delayed” method registration, which means the method will only be registered when the suggested package is loaded.

To use @exportS3Method you must provide the package and generic name in the following format:

It allows to have a tag in a single place, instead of having to make updates in 2 places.

@olivroy
Copy link
Contributor Author

olivroy commented May 19, 2024

I just found the comment I was looking for that advocates for this. r-lib/roxygen2#1534 (comment)

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

Successfully merging this pull request may close these issues.

None yet

3 participants