-
Notifications
You must be signed in to change notification settings - Fork 89
Add tests for quarto, with pdflatex engine #702
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
base: master
Are you sure you want to change the base?
Add tests for quarto, with pdflatex engine #702
Conversation
|
Current solution proposed is working, however, I've listed the following tracks of improvement:
if (length(rd)) {
engine <- pandoc_args[rd + 1]
} else if (is_in_quarto()) {
# retrieve pdf engine if available
engine <- rmarkdown::metadata$format$pdf$`pdf-engine`
if (is.null(engine) || engine == "") {
engine <- "xelatex"
}
} else {
engine <- "pdflatex"
} |
|
Hello @bastienchassagnol, thanks for the PR.
|
|
Hi @eli-daniels — great to hear from you! I’ll do my best to incorporate all the suggested changes. I have two quick comments:
That said, @eli-daniels and @davidgohel, I’d be interested in your thoughts on the |
…minimal changes to latex_str file
| engine <- pandoc_args[rd + 1] | ||
| } else if (is_in_quarto()) { | ||
| engine <- rmarkdown::metadata$`pdf-engine` | ||
| quarto_v <- quarto::quarto_version() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eli-daniels the core changes are brought there.
Depending on the retrieved quarto version, two processes to retrieve metadata of a document:
Sys.getenv("QUARTO_EXECUTE_INFO")since Quarto v1.8, as described in details herermarkdown::metadata, the old-fahsioned way.
| x <- list() | ||
|
|
||
| if (fontspec_compat || is_quarto) { | ||
| if (fontspec_compat) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I insist that variable is_quarto there is not useful. In the first use case, only used for triggering a warning. In the second use case, it's only fontspec_compat, as returned by function get_pdf\engine() that shoudl be used -> indeed, if the rendering is quarto, but the user-defined pdf-engine is pdflatex, you'll add fontspeclatex package dependancy, which is not compliant with the use of pdflatex (will even return the error I've raised in the issue).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eli-daniels (and @davidgohel ) sorry for my delayed answers. I've:
- reverted the formatting changes induced by
airstyler - precisely detail which chnages I have brought to
R/latex_str.R - fetch the latest Quarto changes, and ensure that with my latest changes, rendering with Quarto is compliant with the choice of
pdflatexengine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accordingly, i believe that now, you can finally incporate my suggested PR.
Summary
Add Quarto minimal
reprextests, as the behaviour of Quarto documents differ fromRMarkdownChanges
R/latex_str.Rpaired withR/get_pdf_engineto support Quarto output.tests/testthat/qmd/use-printer-with-pdflatex.qmdRationale
Depending on the
pdf-engineYAML option, you should not loadfontspecpackage -> Inquartoequally toRmarkdown, thepdf-engineshould be processed -> the differences between the two formats mostly lie in the waymetadatafor an individual quarto document is stored, see quarto-dev/quarto-cli#13371 for additional details.Related
Fixes #701
Notes for maintainers
rmarkdown::render, instead of quarto::quarto_render() -> however, doing this, you can't reproduce the error.