tidyfun / tf

S3 classes and methods for tidy functional data
https://tidyfun.github.io/tf/
GNU Affero General Public License v3.0
5 stars 2 forks source link

Move all imports to `R/tf-package.R` #42

Closed m-muecke closed 7 months ago

m-muecke commented 8 months ago

Instead of having the imports in the function roxygen2 I would suggest moving the imports to the R/tf-package.R to remove duplication and make it clear what imports are being used.

fabian-s commented 8 months ago

i don't get it -- that's what @importsFrom etc are for? why do an extra file?

m-muecke commented 8 months ago

i don't get it -- that's what @importsFrom etc are for? why do an extra file?

Currently, there is already a package file https://github.com/tidyfun/tf/blob/dev/R/tf-package.R the difference is only that the roxygen2 import would be in that file instead of the function documentation. Here is an example from the dplyr package: https://github.com/tidyverse/dplyr/blob/main/R/dplyr.R instead of doing it everywhere above the function and from the roxygen docs: https://roxygen2.r-lib.org/articles/namespace.html

The advantage IMO is that in case you have multiple functions using the import it's more centralized. But it's just a design choice. The NAMESPACE file would look the same either way.

fabian-s commented 7 months ago

looking at

https://github.com/search?q=repo%3Atidyverse%2Fdplyr%20importFrom&type=code

it seems like even dplyr still has a bunch of importFrom statements scattered in roxygen-blocks for many functions? not sure what the principle here is.

i actually like the fact that the imports necessary for a function are documented very close to the function code (even if that means we may declare the import of the same function multiple times -- that gets cleaned up by rocygen2 anyway) and don't see any advantage in centralization here...

m-muecke commented 7 months ago

I was more referring to the import and not the importFrom and moving those to tf-package.R basically imports that are used in the entire package.