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

Dependency fixes #14

Closed
wants to merge 8 commits into from

Conversation

gavinsimpson
Copy link
Contributor

This fixes the proximate issue of #9, by adding {lubridate}, {ggplot2}, {sf}, and {sp} as Imports dependencies of {lutz}. As functions from these packages are used via pkg::foo() calls, the respective packages should be in Imports and not Suggests. While one or two functions checked to see if {sf} or {sp} were installed, this is not implemented package-wide. Instead I made the relevant package explicit Imports and further I explicitly imported the used functions from their respective packages via @importFrom statements in the {roxygen} markup.

I also fixed all tractable issues that raised Notes or worse under R CMD check.

Additionally, this PR fixes all failed tests.

Note that this PR won't make {lutz} CRAN-ready; {sp} is about to be deprecated and there are certainly messages about this in the check outputs. I'm not sure what CRAN intends or what is needed for that, but fixing that issue doesn't change the need for the changes addressed here.

@ateucher
Copy link
Owner

Hi, thanks very much for this. As you can see I haven't touched this package in a while.

I don't want to add any required dependencies right now, especially dependencies as heavy as these.

I am actually going to remove the sp methods at some point in the near future (due to the deprecations you mention), and as for ggplot2 I haven't quite decided if I'm going to include the plotting functions in the next release - when I get time to work on this package I will evaluate this and may include ggplot2 in Imports then.

It is not necessary to import functions into the package's namespace via import()/importFrom(), using the package::fun() pattern is a valid and established pattern.

The methods I have used to include optional dependencies are tried and true - using requireNamespace() to check if a package is installed, and prompting the user to install if not.

The current CRAN release is not causing any problems at the moment; rest assured when I prepare the next release I will take all of this into consideration.

@gavinsimpson
Copy link
Contributor Author

No worries. And yes, I see now that you had the requireNamespace() calls. I've cherry-picked out the test and try-related fixes from this PR into two separate PRs to address those small issues.

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.

2 participants