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

Un-deprecate Plumber$run(swaggerCallback=, debug=) and add Plumber$run(docs=,quiet=) #765

Merged
merged 21 commits into from
Mar 1, 2021

Conversation

jcheng5
Copy link
Member

@jcheng5 jcheng5 commented Feb 7, 2021

To better support headless running scenarios. Specifically, I'm creating unit tests for plumbertableau that do not involve using an additional process.

Minimal reproducible example

# Messages printed, swagger UI launched
pr() %>% pr_run()

# No messages printed, no swagger UI launched
pr() %>% pr_run(swaggerCallback = NULL, quiet = TRUE)

PR task list:

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

@jcheng5 jcheng5 marked this pull request as ready for review February 7, 2021 00:40
@meztez
Copy link
Collaborator

meztez commented Feb 7, 2021

This would probably work too

pr() %>% pr_set_docs_callback(NULL) %>% pr_run(quiet = TRUE)

It made realize options_plumber needs to handle deprecated option plumber.swagger.url so we could do

options_plumber(docs.callback = NULL) # Whole session
pr() %>% pr_run(quiet = TRUE)

As it is we still default to it when initializing the router.

I liked the idea of distanciating the package from the swagger ui by using more general names.

@schloerke
Copy link
Collaborator

@jcheng5 @meztez has a fix for the docs.callback option for a global session. (PR fix: #766). I'd prefer to not un-deprecate pr_run(swaggerCallback=) and use pr_set_docs_callback(NULL) instead.


Would Connect ever want access to pr_run(quiet=)? Do you think we should allow users to ever receive the httpuv server to do as they please?

If no, then it seems like an appropriate place to put the quiet= argument (and end of the PR). If yes, then we should prolly make a pr_set_httpuv(quiet = FALSE, sync = TRUE) like method to do this before the pr_run() call.

  • quiet to turn off printing
  • method to switch between httpuv::runServer(), httpuv::startServer(), or return the server before starting. This parameter will take more implementation thought to make sure all existing assumptions (such as any on.exit() call) are covered.

While we do not need to implement the method parameter. I'd like to put the quiet parameter in an appropriate spot with room to grow.

@schloerke schloerke added this to the v1.1.0 milestone Feb 8, 2021
@schloerke
Copy link
Collaborator

Talking more with @wch, the httpuv methods and alterations would prolly be done best with a method outside of pr_run().

Might be good to add a pr_build() which preps the plumber router and pr_run() would wrap pr_build() and then call httpuv::runServer(). For the concept of using different httpuv methods, it would require an update to httpuv such as adding a app$onAppStop(fn) (similar to app$onWSClose()).


tl/dr; Put quiet in pr_run(quiet=) and httpuv related things could be added in new methods.

@meztez
Copy link
Collaborator

meztez commented Feb 11, 2021

Running an headless plumber would make a lot of people happy.

Copy link
Collaborator

@schloerke schloerke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should revert the swaggerCallback changes and also insert ... so that quiet must be full named.

R/plumber.R Outdated
Comment on lines 148 to 150
#' @param swaggerCallback An optional single-argument function that is
#' called back with the URL to an OpenAPI user interface when one becomes
#' ready. If missing, defaults to `$setDocsCallback()`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#' @param swaggerCallback An optional single-argument function that is
#' called back with the URL to an OpenAPI user interface when one becomes
#' ready. If missing, defaults to `$setDocsCallback()`.
#' @param swaggerCallback Deprecated. See `$setDocsCallback()`

R/plumber.R Outdated
#' @importFrom lifecycle deprecated
run = function(
host = '127.0.0.1',
port = getOption('plumber.port', NULL),
swagger = deprecated(),
debug = deprecated(),
swaggerCallback = deprecated()
swaggerCallback,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add in ...

Suggested change
swaggerCallback,
swaggerCallback = deprecated(),
...,

R/plumber.R Outdated
Comment on lines 192 to 193
if (missing(swaggerCallback)) {
swaggerCallback <- private$docs_callback
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (missing(swaggerCallback)) {
swaggerCallback <- private$docs_callback
if (lifecycle::is_present(swaggerCallback)) {
lifecycle::deprecate_warn("1.0.0", "run(swaggerCallback = )", "setDocsCallback(callback = )")
self$setDocsCallback(swaggerCallback)

R/plumber.R Show resolved Hide resolved
R/pr.R Outdated
Comment on lines 478 to 480
#' @param swaggerCallback An optional single-argument function that is called
#' back with the URL to an OpenAPI user interface when one becomes ready. If
#' missing, defaults to `$setDocsCallback()`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#' @param swaggerCallback An optional single-argument function that is called
#' back with the URL to an OpenAPI user interface when one becomes ready. If
#' missing, defaults to `$setDocsCallback()`.

R/pr.R Show resolved Hide resolved
R/pr.R Outdated
@@ -488,11 +492,15 @@ pr_filter <- function(pr,
#' @export
pr_run <- function(pr,
host = '127.0.0.1',
port = getOption('plumber.port', NULL)
port = getOption('plumber.port', NULL),
swaggerCallback,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
swaggerCallback,
...,

R/pr.R Show resolved Hide resolved
R/pr.R Show resolved Hide resolved
NEWS.md Outdated
@@ -23,6 +23,8 @@ plumber 1.0.0.9999 Development version

* OpenAPI Specification can be set using a file path. (@meztez #696)

* Added `quiet = TRUE` to `Plumber$run()` to suppress routine startup messages. (@jcheng5 #765)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Added `quiet = TRUE` to `Plumber$run()` to suppress routine startup messages. (@jcheng5 #765)
* Added `quiet = TRUE` to `pr_run()` and `Plumber$run()` to suppress routine startup messages. (@jcheng5 #765)

* master:
  Add `PlumberEndpoint$setPath(path)` (#770)
  Setting `plumber.docs.callback` to `NULL` will also set legacy `plumber.swagger.url` (#766)
  Update Hosting vignette to swap RStudio Connect and Digital Ocean (#774)
  Use `pak` for GHA (#771)
`debug`, `swaggerCallback`, and `docs`
Add `...` and check that it is empty
@schloerke schloerke changed the title Un-deprecate run(swaggerCallback=), add run(quiet=) param Un-deprecate Plumber$run(swaggerCallback=, debug=) and add Plumber$run(docs=,quiet=) Feb 26, 2021
@schloerke
Copy link
Collaborator

I am going to resolve all requested changes above as they are not to be used.

NEWS.md Outdated Show resolved Hide resolved
R/pr.R Outdated Show resolved Hide resolved
@jcheng5
Copy link
Member Author

jcheng5 commented Mar 1, 2021

LGTM!

@schloerke schloerke merged commit 364fcce into master Mar 1, 2021
@schloerke schloerke deleted the run-headless branch March 1, 2021 22:06
schloerke added a commit that referenced this pull request Mar 1, 2021
* master:
  Un-deprecate `Plumber$run(swaggerCallback=, debug=)` and add `Plumber$run(docs=,quiet=)` (#765)
  Add `PlumberEndpoint$setPath(path)` (#770)
  Setting `plumber.docs.callback` to `NULL` will also set legacy `plumber.swagger.url` (#766)
  Update Hosting vignette to swap RStudio Connect and Digital Ocean (#774)
  Use `pak` for GHA (#771)
  Clarify parameters documentation on when plumber perform type conversion (#756)
  fix brew cask install actions (#761)
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

3 participants