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.

Sign up for GitHub

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

Disallow deploying .qmd files with rmd-* app modes #595

Merged
merged 9 commits into from
Jun 29, 2022
Merged

Disallow deploying .qmd files with rmd-* app modes #595

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e3bced7
Update quarto file handling, add tests
toph-allen Jun 14, 2022
1f19eb8
Fix typo
toph-allen Jun 14, 2022
ad8153a
Adjust language
toph-allen Jun 14, 2022
62e3b4a
The IDE now apparently removes this line from project files
toph-allen Jun 14, 2022
6b1e2aa
Update NEWS.md
toph-allen Jun 14, 2022
7080f6f
Update conditions, adjust tests
toph-allen Jun 15, 2022
02024dc
Remove no-longer-needed test files
toph-allen Jun 15, 2022
478476c
Update Quarto conditions
toph-allen Jun 16, 2022
1626933
Merge branch 'main' into toph-disallow-qmd-as-rmd
toph-allen Jun 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## 0.8.27 (in development)

* Quarto content will no longer silently deploy as R Markdown content when
Quarto metadata is missing or cannot be gathered. Functions will error,
requesting the path to a Quarto binary in the `quarto` argument. (#594)


## 0.8.26

Expand Down
33 changes: 27 additions & 6 deletions R/bundle.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,14 +406,32 @@ inferAppMode <- function(appDir, appPrimaryDoc, files, quartoInfo) {
return("shiny")
}

# Determine if we have Rmd and if they are (optionally) need the Shiny runtime.
rmdFiles <- grep("^[^/\\\\]+\\.[rq]md$", files, ignore.case = TRUE, perl = TRUE, value = TRUE)
# Determine if we have Rmd files, and if they use the Shiny runtime.
rmdFiles <- grep("^[^/\\\\]+\\.rmd$", files, ignore.case = TRUE, perl = TRUE, value = TRUE)
shinyRmdFiles <- sapply(file.path(appDir, rmdFiles), isShinyRmd)

# An Rmd file with a Shiny runtime uses rmarkdown::run.
# Determine if we have qmd files, and if they use the Shiny runtime
qmdFiles <- grep("^[^/\\\\]+\\.qmd$", files, ignore.case = TRUE, perl = TRUE, value = TRUE)
shinyQmdFiles <- sapply(file.path(appDir, qmdFiles), isShinyRmd)

# Trying to deploy Quarto and R Markdown files simultaneously is an error.
if (length(rmdFiles) > 0 && length(qmdFiles) > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should think about this error a bit more. It is possible to have a Quarto project that uses a document.Rmd file. Similarly, it is possible to have a Quarto project with a notebook.ipynb file.

It might be helpful if we captured an enumeration of file name collections and explained which combinations lead to different app modes, especially in the presence / non-presence of quartoInfo.

Here's one example:

  • _quarto.yml
  • analysis.Rmd
  • playground.ipynb

What content type should be used when this code is deployed to Connect? Why?

Similarly, what about:

  • report.Rmd with non-nil quartoInfo

Is this information more a signal to use Quarto or to use R Markdown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you raise a good point.

The app mode is inferred after we have put Quarto info into the format we desire, so by this point it's not clear whether it comes from running quarto inspect or as metadata passed in from the Quarto package.

I'm not sure what Connect would do with the first example you provide.

We had been using quartoInfo's presence as an indicator of user intent to deploy Quarto. If continue to rely on that signal, we could do something like the following.

  • with quartoInfo:
    • App mode is quarto-static, or quarto-shiny if any shinyRmdFiles or shinyQmdFiles.
  • without quartoInfo:
    • App mode is rmd-static or rmd-shiny, as would have been otherwise detected
    • Any qmdFiles or shinyQmdFiles results in an error?

(Thinking about this more. When Quarto renders R Markdown files, their chunk arguments are parsed as R Markdown would, I'm guessing.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, we should discuss tomorrow. I was about to make some changes, but I want to confirm we're on the same page before I do another round on this.

stop("Cannot infer app mode because there are both .qmd and .Rmd files in deployment files.")
}

# To deploy Quarto content, we need to have received or inferred Quarto metadata.
missingQuartoInfoErrorText <- paste(
"Attempting to deploy Quarto project without Quarto metadata.",
"Please provide the path to a quarto binary to the 'quarto' argument."
)

# Shiny or Quarto documents with "server: shiny" in their YAML front matter
# are rmd-shiny or quarto-shiny.
if (any(shinyRmdFiles)) {
return("rmd-shiny")
} else if (any(shinyQmdFiles)) {
if (is.null(quartoInfo)) {
return("rmd-shiny")
stop(missingQuartoInfoErrorText)
} else {
return("quarto-shiny")
}
Expand All @@ -427,10 +445,13 @@ inferAppMode <- function(appDir, appPrimaryDoc, files, quartoInfo) {
return("shiny")
}

# Any non-Shiny R Markdown documents are rendered content (rmd-static).
# Any non-Shiny R Markdown or Quarto documents are rendered content and get
# rmd-static or quarto-static.
if (length(rmdFiles) > 0) {
return("rmd-static")
} else if (length(qmdFiles) > 0) {
if (is.null(quartoInfo)) {
return("rmd-static")
stop(missingQuartoInfoErrorText)
} else {
return("quarto-static")
}
Expand Down
1 change: 0 additions & 1 deletion rsconnect.Rproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ StripTrailingWhitespace: Yes

BuildType: Package
PackageUseDevtools: Yes
PackageCleanBeforeInstall: Yes
PackageInstallArgs: --no-multiarch --with-keep.source
PackageCheckArgs: --as-cran
PackageRoxygenize: rd,collate,namespace
7 changes: 7 additions & 0 deletions tests/testthat/qmd-and-rmd/non-shiny-rmd.Rmd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
title: Not a Shiny R Markdown Document
output: html_document
---



7 changes: 7 additions & 0 deletions tests/testthat/qmd-and-rmd/quarto-doc-none.qmd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
title: "quarto-doc-none"
---

## Quarto

Quarto enables you to weave together content and executable code into a finished document. To learn more about Quarto see <https://quarto.org>.
25 changes: 24 additions & 1 deletion tests/testthat/test-bundle.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,8 +563,31 @@ test_that("writeManifest: Quarto Python-only website gets correct manifest data"
expect_null(manifest$packages)
})

test_that("writeManifest: Deploying Quarto content without Quarto info in an error", {
missingQuartoInfoErrorText <- paste(
"Attempting to deploy Quarto project without Quarto metadata.",
"Please provide the path to a quarto binary to the 'quarto' argument."
)

appDir <- "quarto-website-r"
expect_error(
makeManifest(appDir, appPrimaryDoc = NULL, quarto = NULL),
missingQuartoInfoErrorText
)
})

test_that("writeManifest: Attempting to deploy a directory with Rmd and qmd files is an error", {
quarto <- quartoPathOrSkip()

appDir <- "qmd-and-rmd"
expect_error(
makeManifest(appDir, appPrimaryDoc = NULL, quarto = quarto),
"Cannot infer app mode because there are both .qmd and .Rmd files in deployment files."
)
})

test_that("writeManifest: Sets environment.image in the manifest if one is provided", {
appDir <- "quarto-proj-r-shiny"
appDir <- "shinyapp-simple"

manifest <- makeManifest(appDir, appPrimaryDoc = NULL, image = "rstudio/content-base:latest")
expect_equal(manifest$environment$image, "rstudio/content-base:latest")
Expand Down