Skip to content
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

Guidelines/best practices on exporting adverb-wrapped functions from packages #668

Closed
sheffe opened this issue Apr 25, 2019 · 5 comments · Fixed by #741
Closed

Guidelines/best practices on exporting adverb-wrapped functions from packages #668

sheffe opened this issue Apr 25, 2019 · 5 comments · Fixed by #741
Labels
documentation tidy-dev-day 🤓 Tidyverse Developer Day rstd.io/tidy-dev-day

Comments

@sheffe
Copy link

sheffe commented Apr 25, 2019

insistently() and safely() have been lifesavers for my team, particularly in solving problems where low failure rates over large-scale API call queues and read/write operations would break long-running processes. Interacting with AWS S3 from R is much easier now.

Are there any recommendations/best practices for including adverb-wrapped functions in packages? Since the release of insistently() I’ve never called (eg) the aws.s3::put_object() function directly. Every script starts with creating wrappers for insist_*. I’d like to move the wrapped functions to an internal package, and maybe propose to include an insistent option in the main package. However, I haven’t seen any packages that export safe_* or insistent_* function variants to date, and that’s a good sign I'm planning to do something without full understanding of the side effects.

(First time filing an issue here -- thank you! purrr is the single largest productivity boost I've had in >10 years of writing R code...)

@lionel-
Copy link
Member

lionel- commented Jan 20, 2020

The main thing is to avoid assigning the functions in your namespace at build time. If you assign at build time, this might cause issues when purrr is updated and your package isn't. The functions you have created in the package will contain the old source code, and might even refer to functions that no longer exist in the purrr namespace.

To assign the functions when the package is loaded in memory, do it from the .onLoad() hook of your package:

#' My function
#' @export
insist_my_function <- function(...) "dummy"

my_function <- function(...) {
  # Implementation
}

.onLoad <- function(lib, pkg) {
  insist_my_function <<- purrr::insistently(my_function)
}

@lionel- lionel- added documentation tidy-dev-day 🤓 Tidyverse Developer Day rstd.io/tidy-dev-day labels Jan 20, 2020
@sheffe
Copy link
Author

sheffe commented Jan 22, 2020

Thank you, this is very clear! Using .onLoad for patterns like this is a useful concept in general.

@lawu-mdsol
Copy link

lawu-mdsol commented Jun 25, 2021

@lionel- In your snippet above, why do you define insist_my_function twice?

Also, if I wanted to add additional functionality to the insistently version of a function, e.g. a tryCatch that logs an error, is it still recommended to define this function in the .onLoad() hook? Something like:

.onLoad <- function(lib, pkg) {
  # define insistent version of a function
  insist_s3write_using_func <-purrr::insistently(f = aws.s3::s3write_using)
  
  # wrap function in a trycatch and make it global
  insist_s3write_using <<- function(x, FUN, bucket, object) {
  file <- tryCatch({
    insist_s3write_using_func(x = x,
                              FUN = FUN,
                              bucket = bucket,
                              object = object
    )
  }, error = function(e) {
    print(paste("Unable to write file ", e))
  })
  return(file)
  }
}

@lionel-
Copy link
Member

lionel- commented Jun 28, 2021

why do you define insist_my_function twice?

The dummy definition is a place for the roxygen2 documentation. It also allows some rudimentary static analysis, if only by the readers of the code.

Also, if I wanted to add additional functionality to the insistently version of a function, e.g. a tryCatch that logs an error, is it still recommended to define this function in the .onLoad() hook? Something like:

You can put your wrapper outside of .onLoad(), only the insistently() call needs to be done at load time. You can equivalently call insistently() inside of your wrapper instead of in .onLoad(), if it's only used there. Then the insistent version will be created each time your wrapper is run instead of only once per session. Both approaches will work correctly, the main thing is not to call insistently() at build time.

@lawu-mdsol
Copy link

Thanks @lionel-, that's very helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation tidy-dev-day 🤓 Tidyverse Developer Day rstd.io/tidy-dev-day
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants