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

describe @exportS3Method for non-owned generics and delayed method registration #1087

Merged
merged 7 commits into from
Aug 15, 2024

Conversation

tanho63
Copy link
Contributor

@tanho63 tanho63 commented Aug 15, 2024

Closes #1032 and closes #1082.

Cleans up the section about exporting S3 methods for non-owned generics to recommend using exportS3Method in all scenarios (hard and soft dependencies).

  • For generics owned by hard dependencies (ie Depends and Imports), this previously described a workflow that required importing the generic into the NAMESPACE so that the method could be exported via @export - it seems like this can be skipped and we can use @exportS3Method directly in this scenario.

  • For generics owned by soft dependencies (ie Suggests), this described a methodology around registering S3 methods onLoad(), and suggested an s3_register() function that needed to be included in the package body. This is no longer required as of R 3.6, which now interprets the S3method() directive in NAMESPACE to automatically register the method on dependency load. This means we can use @exportS3Method for this scenario as well, and remove the section on conditional/delayed load entirely.

I'm unsure if it would be worth mentioning how to handle things for pre R3.6 (and hide the deleted section in a collapsed details block?) - R 3.6 is quite old now.

Copy link
Owner

@hadley hadley left a comment

Choose a reason for hiding this comment

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

Nice improvement! I've added a few comments/questions.

dependencies-in-practice.Rmd Outdated Show resolved Hide resolved
dependencies-in-practice.Rmd Outdated Show resolved Hide resolved
dependencies-in-practice.Rmd Outdated Show resolved Hide resolved
dependencies-in-practice.Rmd Outdated Show resolved Hide resolved
dependencies-in-practice.Rmd Outdated Show resolved Hide resolved
dependencies-in-practice.Rmd Outdated Show resolved Hide resolved
Co-authored-by: Hadley Wickham <hadley@posit.co>
dependencies-in-practice.Rmd Outdated Show resolved Hide resolved
@tanho63 tanho63 requested a review from hadley August 15, 2024 18:04
@tanho63 tanho63 requested a review from hadley August 15, 2024 18:15
@hadley hadley merged commit 3663c15 into hadley:main Aug 15, 2024
@hadley
Copy link
Owner

hadley commented Aug 15, 2024

Thank you!

@tanho63 tanho63 deleted the tan/delayed-s3-registration branch August 15, 2024 19:21
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.

Update the section about conditional / delayed S3 registration Update S3 method advice
2 participants