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

Interface mount #562

Closed
wants to merge 65 commits into from
Closed

Interface mount #562

wants to merge 65 commits into from

Conversation

meztez
Copy link
Collaborator

@meztez meztez commented Jun 23, 2020

Decouple ui mounting from the run method and generalize interface

PR task list:

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

Bruno Tremblay added 3 commits June 23, 2020 09:20
Merge remote-tracking branch 'upstream/master' into interface_mount

# Conflicts:
#	R/plumber.R
#	man/plumber.Rd
@meztez meztez mentioned this pull request Jun 23, 2020
30 tasks
R/ui.R Show resolved Hide resolved
R/ui.R Outdated Show resolved Hide resolved
R/ui.R Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
scripts/manual_testing.R Outdated Show resolved Hide resolved
R/ui.R Outdated Show resolved Hide resolved
R/ui.R Outdated Show resolved Hide resolved
R/ui.R Outdated Show resolved Hide resolved
R/ui.R Outdated Show resolved Hide resolved
R/ui.R Outdated Show resolved Hide resolved
R/ui.R Outdated Show resolved Hide resolved
R/ui.R Outdated Show resolved Hide resolved
R/ui.R Outdated Show resolved Hide resolved
R/ui.R Outdated Show resolved Hide resolved
@schloerke
Copy link
Collaborator

schloerke commented Jun 23, 2020

Lots of small changes. Trying to future-safeguard external "contracts" with other packages as we won't be able to update them when we want.

meztez and others added 8 commits June 23, 2020 14:42
Co-authored-by: Barret Schloerke <barret@rstudio.com>
Co-authored-by: Barret Schloerke <barret@rstudio.com>
Merge remote-tracking branch 'upstream/master' into interface_mount

# Conflicts:
#	DESCRIPTION
#	R/plumber.R
#	man/plumber.Rd
@meztez meztez requested a review from cpsievert July 3, 2020 15:39
Bruno Tremblay added 2 commits July 4, 2020 08:44
Merge remote-tracking branch 'upstream/master' into interface_mount

# Conflicts:
#	R/plumber.R
R/plumber.R Outdated Show resolved Hide resolved
R/plumber.R Outdated Show resolved Hide resolved
R/plumber-options.R Outdated Show resolved Hide resolved
Bruno Tremblay and others added 2 commits July 13, 2020 15:26
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
R/plumber.R Outdated Show resolved Hide resolved
R/plumber.R Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@cpsievert cpsievert self-requested a review July 13, 2020 20:06
Copy link
Contributor

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

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

LGTM pending comments

R/plumber.R Outdated
swaggerCallback = getOption('plumber.swagger.url', NULL)
swaggerCallback,
...,
callback = getOption('plumber.ui.callback', getOption('plumber.swagger.url', NULL))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Received more feedback from the Connect team. I'd like to remove this argument and make it a R6 router method.

The default return value for the R6 method should still be getOption('plumber.swagger.url').

I'd also like to do this for debug. Where the default value is interactive().

This makes it so the $run() method has NOTHING in the parameters except for host and port.

Copy link
Collaborator Author

@meztez meztez Jul 17, 2020

Choose a reason for hiding this comment

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

I've done as you suggested.
Also added a callback argument to setUI and added a setDebug method.

Note : @cpsievert previously requested to add back the arguments to the run method as they were removed originally.

@schloerke schloerke added the Fixed in PR A PR has implemented a fix for this Issue label Jul 20, 2020
@meztez meztez closed this Jul 20, 2020
@muschellij2
Copy link
Contributor

It looks like

set_ui = function(
was changed to set_ui but still uses setUI in
ExecStart=/usr/bin/Rscript -e "pr <- plumber::plumb('/var/plumber$PATH$/plumber.R'); pr$setUI($SWAGGER$); $PREFLIGHT$ pr$run(port=$PORT$)"

@schloerke
Copy link
Collaborator

@muschellij2 Fixed in 69e5011. 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed in PR A PR has implemented a fix for this Issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants