NOAA-FIMS / FIMS

The repository for development of FIMS
https://noaa-fims.github.io/FIMS/
GNU General Public License v3.0
13 stars 8 forks source link

Feedback needed: Decide on R function importing style #217

Closed k-doering-NOAA closed 2 years ago

k-doering-NOAA commented 2 years ago

Note: please thumbs up or thumbs down this comment if you agree/disagree with the style outlined here. Please also feel free to post what parts you like/dislike.

This Pull request brought up some questions about the style of importing packages (or functions within packages) for use within the FIMS R package. The options are:

  1. Calling the function with :: within the FIMS function where it is needed, e.g., mypkg::myfunction()
  2. Calling the function with @import mypkg or @importFrom mypkg myfunction so that roxygen will automatically add either the whole package or the function to the NAMESPACE file.

Solution: we could Follow the google style guide generally [our exceptions noted in brackets]

Users should explicitly qualify namespaces for all external functions. purrr::map()# Good

We discourage using the @import Roxygen tag to bring in all functions into a NAMESPACE. Google has a very big R codebase, and importing all functions creates too much risk for name collisions.

While there is a small performance penalty for using ::, it makes it easier to understand dependencies in your code. There are some exceptions to this rule.

  • Infix functions (%name%) always need to be imported.
  • Certain rlang pronouns, notably .data, need to be imported. [note: we would import this from ggplot2]
  • Functions from default R packages, including datasets, utils, grDevices, graphics, stats and methods. If needed, you can @import the full package.
  • When importing functions, place the @importFrom tag in the Roxygen header above the function where the external dependency is used. [@kellijohnson-NOAA suggested instead having a single file where we put @import or @importFrom]

Another point is that ggplot2 code can look a bit messy, so we could use @import ggplot2. It also is already clear from the unique syntax what the package is. Edited: Following recommendations for using ggplot2 in Rpackages, ggplot2:: should be used for all functions opposed to @import ggplot2

iantaylor-NOAA commented 2 years ago

If we follow the proposed solution and then change our mind, it's really easy to switch from :: to @import or @importFrom. Going from @import to :: requires more work to figure out which functions from which packages have been used.

JaneSullivan-NOAA commented 2 years ago

Sorry for the slow response, Kathryn. I prefer a hybrid approach, where routinely used packages (e.g. ggplot2) are imported at the top of the function but less commonly used packages are called out with ::. Additionally, packages like dplyr with function names that are duplicated in other packages should probably (unfortunately) use ::

k-doering-NOAA commented 2 years ago

Thanks Jane, @Andrea-Havron-NOAA found this out after opening the issue (this is in the edited original issue, but reposting here for clarity):

Edited: Following recommendations for using ggplot2 in Rpackages, ggplot2:: should be used for all functions opposed to @import ggplot2

nathanvaughan-NOAA commented 2 years ago

Now this popped back up again I would side with the :: approach. I think it is helpful to know explicitly where a function is coming from for debugging, review, and any future changes.

timjmiller commented 2 years ago

I also vote for using ::

ChristineStawitz-NOAA commented 2 years ago

Resolution: using :: except with the exceptions noted above in the Google style guide - all @import tags when needed are in fims-package.R file