Revise quarto argument #828
Conversation
I'm assuming that the previous interface of supplying a path is mostly an accident of history (not a desired UI) and hasn't been used to select between different versions of quarto. Fixes #658
There was a problem hiding this comment.
@toph-allen - do you still have some of the example/testing workflows from when we last adjusted Quarto support?
| @@ -275,6 +274,14 @@ appIsDocument <- function(appMode) { | |||
| ) | |||
| } | |||
|
|
|||
| appIsQuartoDocument <- function(appMode) { | |||
There was a problem hiding this comment.
nit: Because this might be a single document or a more complicated project; maybe appIsQuartoContent?
There was a problem hiding this comment.
Words are hard, eh? Other thoughts: isQuartoContent, appModeIsQuarto, etc.
There was a problem hiding this comment.
I just modified from the appIsDocument() above, so while I agree that the name isn't the best, I'll leave it as for this PR.
R/deployApp.R
Outdated
| @@ -227,6 +231,15 @@ deployApp <- function(appDir = getwd(), | |||
| check_file(recordDir) | |||
| } | |||
|
|
|||
| if (!is_string(quarto)) { | |||
There was a problem hiding this comment.
Is this check negated? Warn on an incoming string.
There was a problem hiding this comment.
Looks good to me, assuming fixes to the things @aronatkins pointed out. (And there's a broken test on devel.)
When we introduced Quarto support, we tested using items from https://github.com/rstudio/connect-content. We have a pretty full set of Quarto testing content there. Assuming we want to QA this against Quarto content and non-Quarto content without specifying quarto as TRUE or FALSE, we'd do something like the following, from the connect-content/bundles directory.
library(rsconnect)
# Quarto content
deployApp("quarto-book-none")
deployApp("quarto-doc-none")
deployApp("quarto-multidoc-proj-none")
deployApp("quarto-proj-none")
deployApp("quarto-proj-none-ojs")
deployApp("quarto-proj-py")
deployApp("quarto-proj-r")
deployApp("quarto-proj-r-py")
deployApp("quarto-proj-r-shiny")
deployApp("quarto-website-none")
deployApp("quarto-website-py")
deployApp("quarto-website-r")
deployApp("quarto-website-r-py")
deployApp("quarto-website-r-py-separate-files")
# Non-quarto content
deployApp("rmd-shiny-runtime-shinyrmd-complete")
deployApp("rmd-shiny-runtime-shinyrmd-simple")
deployApp("rmd-site")
deployApp("shinyapp")| @@ -275,6 +274,14 @@ appIsDocument <- function(appMode) { | |||
| ) | |||
| } | |||
|
|
|||
| appIsQuartoDocument <- function(appMode) { | |||
There was a problem hiding this comment.
Words are hard, eh? Other thoughts: isQuartoContent, appModeIsQuarto, etc.
| #' @param quarto Should the deployed content be built by quarto? | ||
| #' (`TRUE`, `FALSE`, or `NA`). The default, `NA`, will use quarto if | ||
| #' there are `.qmd` files in the bundle, or if there is a | ||
| #' `_quarto.yml` and `.Rmd` files. | ||
| #' | ||
| #' (This option is ignored and quarto will always be used if the | ||
| #' `metadata` contains `quarto_version` and `quarto_engines` fields.) |
There was a problem hiding this comment.
I don't fully remember why we required a Quarto path originally. I think that @aronatkins and I discussed not wanting to make assumptions that would break existing workflows, so we kept Quarto support gated behind the parameter? At this point, I definitely prefer this approach.
I'm assuming that the previous interface of supplying a path is mostly an accident of history (not a desired UI) and hasn't been used to select between different versions of quarto.
Fixes #658