ateucher / lutz

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

{sf} is not Imported by {lutz} #9

Closed adamhsparks closed 1 year ago

adamhsparks commented 3 years ago

This can cause issues when {lutz} is imported by another package and uses functionality in {lutz} that depends on the {sf} package. As a result of {sf} only being in Suggests, tests fail and Notes appear when running checks on the dependent package, which caused me a bit of grief for a bit until I figured out what was going on and recalled a similar issue with another package that {bomrang} uses.

I've implemented a workaround in our package that Imports {lutz}.

# This function is never called.
# It only makes `sf` available in the package for `lutz` to use.
# `lutz` only has `sf` in Suggests, but it is needed for the function we use
# from `lutz`

stub <- function() {
   sf::st_as_sf()
}

Suboptimal, but works without any major side-effects and all tests pass and {sf} is sure to be installed if it's not on the machine that installs our package, etc.

Ideally, {sf} should be imported by {lutz} to avoid workarounds like this.

ateucher commented 3 years ago

Thanks @adamhsparks I'll do this when I next crack lutz open for maintenance.

gavinsimpson commented 1 year ago

I just hit this too; would a PR help?

ateucher commented 1 year ago

Hi, thanks for the renewed attention on this! I don't think I will add sf to Imports as it is a very heavy, and optional, dependency. If you need that functionality in your package, adding sf to your package imports and including a "stub" function like @adamhsparks is the right way to do it: https://r-pkgs.org/dependencies-in-practice.html#how-to-not-use-a-package-in-imports.

gavinsimpson commented 1 year ago

No worries, but then I think you should use anything from Suggests selectively, checking by requireNamespace or similar if a package is available before continuing with code. and you're already doing the right thing with requireNamespace().

Something is getting messed up with dependencies not being installed or loading on GH Actions with the rlibs Actions and lutz, and adding a stub to force loading a package you're using isn't expected behaviour for something that is the functionality in the package name :) I've implemented the the stub fix and will see if that fixes the GH Actions issue and continue digging as there is certainly something wrong with dependency resolution with the Action.

ateucher commented 1 year ago

isn't expected behaviour for something that is the functionality in the package name

I get where you're coming from here, but sf is a heavy dependency which can be tricky to install, and only required if you're using the "accurate" method. The "fast" method is very good and will be enough for many users. Have you compared the two methods on the stations your package retrieves? If the "fast" method is sufficient, then you can also avoid imposing sf on your users.

adamhsparks commented 1 year ago

For me it wasn't about which method was "good enough", it's an issue passing CRAN checks and GH actions, etc. since the package isn't properly loaded.

gavinsimpson commented 1 year ago

Have you compared the two methods on the stations your package retrieves? If the "fast" method is sufficient, then you can also avoid imposing sf on your users.

I considered it, but then I saw #12 and had second thoughts so didn't pursue that option. Ideally I'd have this {sf} dependence on {canadaHCD} in Suggests (as I thought plotting a station map or finding stations using a map would be a nice addition but not necessary, and I only need {sf} currently whenever someone updates the station list in the pkg) so I'll look into that. But as @adamhsparks says something is breaking with dependencies and the setup-r-dependencies Action that I haven't gotten to the bottom of. Will try to pull a MWE package to reproduce the issue and follow up with the Action authors.

I'll also look at whether the fast option works for my specific application.

Thanks for the suggestion @ateucher

ateucher commented 1 year ago

I've created a minimal package that depends on {lutz} and the use of the "accurate" method here, with the approach of ignore_unused_imports() + including {sf} in Imports here. The GHA seem to be working, but it's possible I've missed a nuance of what you are trying to do. Let me know!

gavinsimpson commented 1 year ago

Yeah, that works (and it's what I have working in {canadaHCD}. But what @adamhsparks and I are talking about is the situation where you don't include {sf} as any kind of dependency of this new skeleton package. I.e. remove {sf} from DESCRIPTION and then the GH actions will fail running tests involving {lutz} functions with errors saying that {sf} in not available.

ateucher commented 1 year ago

If you're using a lutz function that requires the use of sf, and not including sf in Imports, then yes, that's expected, as putting sf in Imports in your package is the only way to ensure your user (or GHA) has it installed - since lutz does not do that.

If you're using a lutz function that doesn't require sf then you shouldn't need sf in Imports (or the stub function).

ateucher commented 1 year ago

Regarding #12, I will still support some form of fast method bundled with the package (ie not requiring sf), I just need to choose one of the many V8/Rust/C implementations to use. It will still likely be less accurate due to size constraints if the underlying data.

ateucher commented 1 year ago

I think I can close this now - if the solutions discussed above aren't working, please let me know and we can reopen it.