Handle missingness

[coatless]

Consider the use of NA and NULL being passed into a named parameter:

gh("/repos/:owner/:repo/issues", owner = "r-lib", repo = "gh", state = NULL)
# Error in gsub(paste0(":", n, "\\b"), p, endpoint) : 
#  invalid 'replacement' argument

gh("/repos/:owner/:repo/issues", owner = "r-lib", repo = "gh", state = NA)
# Error in gh("/repos/:owner/:repo/issues", owner = "r-lib", repo = "gh",  : 
#  GitHub API error (422): 422 Unprocessable Entity
#  Validation Failed

This is a bit problematic for parameters that may be optional. (c.f. coatless-rpkg/ghapi3#3)

Would it be possible to add filtering inside of gh_set_endpoint() in R/gh_request.R to remove parameters that have R-specific missingness? e.g.

gh_set_endpoint <- function(x) {
  params <- x$params
  if (!grepl(":", x$endpoint) || length(params) == 0L || has_no_names(params)) {
    return(x)
  }

  # Subset out invalid params
  params <- Filter(Negate(is.null), params)
  params <- Filter(Negate(is.na), params)

  # rest of the logic ... 
}

[jennybc]

Related to #21 (which is a pretty old issue, perhaps unhelpfully old)

[coatless]

Reading #21 (comment), it seems like the goal should be to trigger an error in the endpoint instead of silently removing the missingness components as-is described above. So, the following should be added:

param_names = names(params)
missingness_params = param_names[sapply(params, function(x) is.null(x) || is.na(x)] 

if(length(missingness_params) > 0) {
   msg = paste("Parameters must not have `NA` or `NULL` as their values.",
                         "The following parameters were detected to have some level of missingness: \n"
                     paste0(missingness_params, collapse = "\n"))
   stop(msg, call. = FALSE)
}

Though, that leaves the case brought up in coatless-rpkg/ghapi3#3 that requires flexibility of defined parameters to act as optional criteria missingness. I suppose the best way to handle this is to retrieve only valid parameters prior to the gh() call. Thus, any named element with missingness is deleted using a custom cleaning function and, then, the new list of parameters would then be passed togh() via do.call().

valid_params = function(...) {
  params = list(...)
  params = Filter(Negate(is.null), params)
  params = Filter(Negate(is.na), params)
  
  params
}

repo_transfer = function(owner, repo, new_owner, team_id = NA) {

  params = valid_params(endpoint = "custom endpoint here", owner = owner, repo = repo, new_owner = new_owner, team_id = team_id)
  
  params
  
  ## Call the gh function dynamically with the subset of parameters potentially
  # do.call(gh, params)
  
}

[lwjohnst86]

I bypassed that here using a combination of:

add_request_parameters <- function(...) {
    params <- list(...)
    Filter(Negate(is.null), params)
}

Each param's default is NULL. Then it is passed to:

do.call(gh::gh, params)

[gaborcsardi]

I am planning to fix this by

1. dropping named NULL parameters silently, so NULL means "do add this query parameter", and
2. throwing an error on named NA parameters.

Non-named parameters are not touched, since they go into the request body, they will be JSON encoded, and NA and NULL are valid here. (Weird, but valid.)

[coatless]

@gaborcsardi awesome. Do you have an ETA on when the next version of gh will go onto CRAN?

[gaborcsardi]

today