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

DRY: factoring out the argument cleanup that many rio.import_* and rio.export_* functions need. #245

Closed
1 of 3 tasks
bokov opened this issue Oct 28, 2019 · 2 comments
Closed
1 of 3 tasks
Milestone

Comments

@bokov
Copy link
Contributor

bokov commented Oct 28, 2019

Please specify whether your issue is about:

  • a possible bug
  • a question about package functionality
  • a suggested code or documentation change, improvement to the code, or feature request

I would like to volunteer to fix #241 (actually I meant to submit a PR after I posted it, but got busy with a few other things). Looking over it I see an opportunity to simplify the individual import and export functions and make them more maintainable. Namely generalizing the args <- args[intersect(args,formals(fn))] pattern in the function sketched out below (not yet thoroughly tested, just proof of concept). While we're at it, we could include additional safeguards: removing duplicate arguments and unnamed arguments, and re-mapping certain argument names. This function could safely be run on all underlying functions-- if they don't need argument filtering because they have ... among their formals, they would still benefit from remapping and protection against duplication (to which ... functions are particularly vulnerable otherwise).

I am not proposing to cut over all the functions at once. I am proposing to use this for fixing current and future unknown-argument bugs as they are noticed. Possibly updating and testing and already patched functions such as in #221 and #223 individually as time permits.

I understand reluctance to introduce an upstream dependency on R.utils::DoCall which has similar functionality. The below code only uses base R.

So, if this is acceptable, I would like to submit this as a PR, and follow that one with a PR that actually closes #241

Put your code here:

#' Adapt an argument list to a function excluding arguments that will not be 
#' recognized by it, redundant arguments, and un-named arguments.
#'
#' @param fn A function to which an argument list needs to be adapted.
#' @param ... An arbitrary list of named arguments (unnamed ones will be ignored)
#' @param .args A list or \code{alist} of named arguments, to be merged with \code{...}
#' @param .docall If set to \code{TRUE} will not only clean up the arguments but 
#'                also execute \code{fn} with those arguments (\code{FALSE} by
#'                default)
#' @param .remap An optional named character vector or named list of character 
#'               values for standardizing arguments that play the same role but
#'               have different names in different functions.
#' @param .warn Whether to issue a warning message (default) when invalid 
#'              arguments need to be discarded.
#'
#' @return Either a named list or the result of calling \code{fn} with the 
#'         supplied arguments
#'
#' @examples
#' 
#' arg_reconcile(p.adjust,bla='aaa',baz='xzzz',n=40,q=c(.1,.2,.02,.3,1,0,1),.remap = c(q='p'))
#' arg_reconcile(import,file='hello.csv',file='goodbye.csv',
#'               bla='aaa',baz='xzzz',format='csv')
arg_reconcile <- function(fn,...,.args=alist(),.docall=FALSE,
                          .remap=list(),
                          .warn=TRUE){
  # capture the formal arguments of the target function
  frmls <- formals(fn)
  # capture the arguments, both freeform and an explicit list
  args <- c(list(...),.args)
  # get rid of duplicate arguments
  for(ii in names(args)) {
    if( sum(dupe <- names(args) %in% ii)>1) {
      args[seq_along(args)[dupe][-which(TRUE,dupe)]] <- NULL
    }
  }
  # if any remappings of one argument to another are specified, perform them
  for( ii in names(.remap) ){
    if( !.remap[[ii]] %in% names(args) && ii %in% names(args) ){
      args[[.remap[[ii]] ]] <- args[[ii]]}
  }
  # remove any unnamed arguments
  args[names(args)==""] <- NULL
  # if the target function doesn't have '...' as an argument, check to make sure
  # only recognized arguments get passed, optionally with a warning
  if ( !'...' %in% names(frmls) ){
    unused <- setdiff(names(args),names(frmls))
    if ( length(unused)>0 && .warn ){
      warning("The following arguments were ignored for ",substitute(fn),":\n",
              paste(unused, collapse = ', '))
      args <- args[intersect(names(args),names(frmls))]
    }
  }
  # the final, cleaned-up arguments either get used on the function or returned
  # as a list, depending on how .docall is set
  if ( .docall ) return(do.call(fn,args)) else return(args);
}
@bokov bokov changed the title DRY: factoring out the argument cleanup that many rio.import_* functions need. DRY: factoring out the argument cleanup that many rio.import_* and rio.export_* functions need. Oct 28, 2019
@leeper leeper added this to the v0.6 milestone Dec 20, 2019
bokov added a commit to bokov/rio that referenced this issue Dec 20, 2019
@bokov bokov mentioned this issue Dec 20, 2019
6 tasks
@bokov
Copy link
Contributor Author

bokov commented Mar 2, 2020

Now have full test coverage of the arg_reconcile() function in latest #251

@bokov
Copy link
Contributor Author

bokov commented Mar 8, 2021

The merge of PR #251 has achieved this.

Inclusion of code from PRs #255 #256, and #257 has in my opinion has accomplished argument cleanup for the following formats:

  • xls
  • xlsx
  • dta
  • Formats handled by import_delim

There are additional import functions that would benefit from arg_reconcile but I will put them in a separate ticket

@bokov bokov closed this as completed Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants