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

Better support for enhancing packages e.g. ledger #335

Open
chainsawriot opened this issue Sep 3, 2023 · 3 comments
Open

Better support for enhancing packages e.g. ledger #335

chainsawriot opened this issue Sep 3, 2023 · 3 comments

Comments

@chainsawriot
Copy link
Collaborator

ledger by @trevorld has support for rio, even though it is not in rio's Suggests. And of course, the code is not in this package. Currently, rio raises error with this message

xA <- gettext("Import support for the %s format is exported by the %s package. Run 'library(%s)' then try again.")

A better approach for this is to first check for installation using .check_pkg_availability. If it's available, library() that package (or test with directly using the S3 method) and then import again. If it is not available, error the same way and suggest installing that package.

It can also be documented in the Vignette as a possible way to expand rio #320 .

@trevorld if you are reading this, you can put rio in Enhances.

@trevorld
Copy link
Contributor

trevorld commented Sep 3, 2023

Relevant history:

requireNamespace("ledger", quietly = TRUE) I think would be the preferred way to test if {ledger} is installed and if it is load it so the S3 methods work...

@chainsawriot , IMO I think {rio} belongs in {ledger}'s Suggests field instead of Enhances because I test my the {rio} functionality in {ledger}'s tests and according to documentation all packages tested by tests should be in Suggests (if they aren't already in Imports or Depends). Also Suggests packages are installed by install.packages(dependencies = TRUE) whereas users need to manually specify Enhances packages in a custom character vector if they want to install them. I think historically a long time ago CRAN borrowed the Enhances field from Debian package format but in practice Enhances seems to be pretty rare and ignored by popular tools in both software distribution platforms...

@trevorld
Copy link
Contributor

trevorld commented Sep 3, 2023

I wildly speculate that if in an .onLoad() hook you requireNamespace() on all known packages that register S3 methods for {rio} then those import() / export() S3 methods would work if {rio} was loaded by the user/developer without the user/developer needing to explicitly load those known packages. However loading packages my have side effects so maybe you wouldn't want to do that.

@trevorld
Copy link
Contributor

trevorld commented Sep 4, 2023

@chainsawriot I see you are the new maintainer. A previous maintainer @leeper did not want to add {ledger} to {rio}'s optional Suggests field because {ledger} has a SystemRequirements field (suggestions really, the package will install fine if some/all aren't installed but at run-time you need the right program installed for the right file format).

I'm okay with registering S3 import methods for these file formats directly in {rio} instead of registering them in {ledger} but if so you'll probably want to add {ledger} to your Suggests...

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

No branches or pull requests

2 participants