search

ateucher / lutz

Look up timezones of point coordinates
http://andyteucher.ca/lutz/
Other
58 stars 5 forks source link

Dependency fixes #14

Closed gavinsimpson closed 1 year ago

gavinsimpson commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.