Check if some required packages could be made suggested

epiforecasts / covidregionaldata

An interface to subnational and national level COVID-19 data. For all countries supported, this includes a daily time-series of cases. Wherever available we also provide data on deaths, hospitalisations, and tests. National level data is also supported using a range of data sources as well as linelist data and links to intervention data sets.

https://epiforecasts.io/covidregionaldata/

Other

37 stars 18 forks source link

Check if some required packages could be made suggested #434

Closed RichardMN closed 2 years ago

RichardMN commented 2 years ago

Working through the new Colombia data (#432 and #433) we started getting a warning about importing too many packages. For packages which are only necessary for a few ("few" not yet defined) specific datasets, we could make these suggested imports instead of required and box around them, either:

to have the package try to install a package, or
to warn that a package is necessary and fail, or
to work around the gap (as is possible in #433)

This will first require looking at which files are using which imported functions and identify which may be candidates for pruning. Suggestions on tools to do this smoothly would be welcome!

seabbs commented 2 years ago

Hi Richard,

Firstly sorry for the lack of response here recently been very busy on another project and neglected everything else. Will get up to speed on the work you have been doing over the next few days and try and make some useful contributions.

For this issue, I agree limiting imports is a really good idea. My preferred solution would be yes to go with suggests and allow then a check for the package with a message about what is needed to be installed.

Auto-installing would make CRAN not happy I think and introducing code work arounnds might just make it harder to code things (though if possible could be useful).

Bisaloo commented 2 years ago

I think this rule of < 20 dependencies is counterproductive. I predicted people will just start depending on big meta-packages (like tidyverse) instead of smaller, more focused packages.

I suggest we do something like this (but not as hacky): many functions of the tidyverse are re-exported in many places and we can import them in a clever way to minimise dependencies. For example, tidyselect::any_of(), tidyselect::ends_with(), etc. are re-exported in dplyr so we can depend on dplyr only instead of tidyselect+dplyr.

I can submit a PR for this later this week.

RichardMN commented 2 years ago

If you can figure out why we're getting CMD check failures in reexports.Rd that would be welcome too. My local build (when I build the documentation) makes a pointer to \item{rlang}{\code{\link[rlang:dot-data]{.data}}} which fails; when I change it to \item{rlang}{\code{\link[rlang:tidyeval-data]{.data}}} it works, but I am now having to do this manually after building the documentation.

Bisaloo commented 2 years ago

This was changed in the development version of rlang: https://github.com/r-lib/rlang/commit/aa587b037adb4b3b3923024f5711cce0f14d86dd#diff-e4299ca7439da3d937d0a2b7b28e948378c040e1c337911055dbddf60af08e5c so it's probably just a mismatch between your local version and the version on GitHub Actions.

I don't think this was intended to be a re-export anyways so I removed it in https://github.com/epiforecasts/covidregionaldata/pull/437/commits/d3c150b155c1f56689f7c380c9c952db7941e3c3.