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

Tidy plumber API #590

Merged
merged 28 commits into from
Jul 22, 2020
Merged

Tidy plumber API #590

merged 28 commits into from
Jul 22, 2020

Conversation

blairj09
Copy link
Collaborator

@blairj09 blairj09 commented Jul 9, 2020

First pass at a "tidy" API for the R6 methods used by plumber. Tests still need to be added, but wanted to get this initial approach out there.

PR task list:

  • Update NEWS
  • Add tests
  • Update documentation with devtools::document()

@blairj09 blairj09 changed the title Pr pipes Tidy plumber API Jul 9, 2020
R/pr.R Outdated Show resolved Hide resolved
R/pr.R Outdated Show resolved Hide resolved
R/pr.R Outdated Show resolved Hide resolved
R/pr.R Outdated Show resolved Hide resolved
R/pr.R Outdated Show resolved Hide resolved
R/pr.R Outdated Show resolved Hide resolved
R/pr.R Outdated Show resolved Hide resolved
R/pr.R Outdated Show resolved Hide resolved
R/pr.R Outdated Show resolved Hide resolved
R/pr.R Outdated Show resolved Hide resolved
Co-authored-by: Barret Schloerke <barret@rstudio.com>
@schloerke
Copy link
Collaborator

  • Warning being produced
* checking Rd \usage sections ... WARNING
##[warning]Undocumented arguments in documentation object 'pr_handle'
  ‘serializer’

Undocumented arguments in documentation object 'pr_register_hook'
  ‘handlers’

@schloerke
Copy link
Collaborator

Final concern is over pr_new(). The name implies something that is only specific to R6. I understand the concept of where it came from. But it feels weird.

Ideas for a different name...

  • plumb() (Allow for no file or directory information)
  • pr_init()
  • plumber() Can't double use the variable name. 😞
  • plumbr() (Too close / confusing)
  • pr()
  • plmbr()

@meztez
Copy link
Collaborator

meztez commented Jul 10, 2020

I like pr()

@schloerke
Copy link
Collaborator

Talked with @blairj09 . pr() it is! Has a nice auto complete pairing with pr_*()

@schloerke
Copy link
Collaborator

schloerke commented Jul 14, 2020

@blairj09 Can we also add:

pr_cookie <- function(
  pr,
  key,
  name = "plumber",
  expiration = FALSE,
  http = TRUE,
  secure = FALSE
) {
  pr$registerHooks(
    sessionCookie(key = key, name = name, expiration = expiration, http = http, secure = secure)
  )
  invisible(pr)
}

The tests look great!

@schloerke schloerke added this to the v0.5.0 - Next CRAN release milestone Jul 15, 2020
@schloerke
Copy link
Collaborator

schloerke commented Jul 15, 2020

Have another addition (via @trestletech) from https://www.rplumber.io/articles/security.html#cross-origin-resource-sharing-cors

This one is also a similar situation to pr_cookie() in that it provides a clean / common interface.

(** Update)
Breaking a part the top comment here:

#' @filter cors
cors <- function(req, res) {
  
  res$setHeader("Access-Control-Allow-Origin", "*")
  
  if (req$REQUEST_METHOD == "OPTIONS") {
    res$setHeader("Access-Control-Allow-Methods","*")
    res$setHeader("Access-Control-Allow-Headers", req$HTTP_ACCESS_CONTROL_REQUEST_HEADERS)
    res$status <- 200 
    return(list())
  } else {
    plumber::forward()
  } 
}

into

pr_cors <- function(pr, origin, name = "cors") {
  pr_filter(pr, name, function(req, res) {
    res$setHeader("Access-Control-Allow-Origin", origin)
    forward()
  })
}
pr_options <- function(pr, methods = "*", name = "options") {
  pr_filter(pr, name, function(req) {
    if (req$REQUEST_METHOD == "OPTIONS") {
      res$setHeader("Access-Control-Allow-Methods","*")
      res$setHeader("Access-Control-Allow-Headers", req$HTTP_ACCESS_CONTROL_REQUEST_HEADERS)
      res$status <- 200 
      return(list())
    }
 
    plumber::forward()
  })
}

@schloerke schloerke requested a review from cpsievert July 17, 2020 14:47
R/pr.R Outdated Show resolved Hide resolved
R/pr.R Outdated Show resolved Hide resolved
R/pr.R Outdated Show resolved Hide resolved
R/pr.R Outdated Show resolved Hide resolved
R/pr.R Show resolved Hide resolved
R/pr.R Show resolved Hide resolved
R/pr.R Show resolved Hide resolved
R/pr.R Show resolved Hide resolved
R/pr.R Show resolved Hide resolved
R/pr.R Show resolved Hide resolved
- Standardize capitalization
- Add checks for pr object
- Minor documentation updates
- POST_BODY -> postBody
pkgdown/_pkgdown.yml Outdated Show resolved Hide resolved
@schloerke schloerke merged commit 6d6e528 into rstudio:master Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants