-
Notifications
You must be signed in to change notification settings - Fork 5
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
{sf} is not Imported by {lutz} #9
Comments
Thanks @adamhsparks I'll do this when I next crack lutz open for maintenance. |
I just hit this too; would a PR help? |
Hi, thanks for the renewed attention on this! I don't think I will add |
No worries, 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. |
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. |
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. |
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 I'll also look at whether the fast option works for my specific application. Thanks for the suggestion @ateucher |
I've created a minimal package that depends on {lutz} and the use of the "accurate" method here, with the approach of |
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 |
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). |
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. |
I think I can close this now - if the solutions discussed above aren't working, please let me know and we can reopen it. |
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}.
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.
The text was updated successfully, but these errors were encountered: