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

Fix embed and verbatim knitr's engine for revealjs #4732

Merged
merged 7 commits into from
Mar 10, 2023

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Mar 9, 2023

General fix : With new embed feature in 1.3 for Jupyter, we need to make sure this does not override embed Knitr engine (cc @dragonstyle as discussed)

Specific to revealjs: Set echo = TRUE for embed and verbatim engine when revealjs because those engine expect echo to be TRUE to be shown, but revealjs default to FALSE

This fix #4712

Note: our extension to include external file works fine - but it feels like those engines should still work correctly

By working on these R files, I think we can improve the handling of chunk hook, especially to treat those two engines as others (if we want to support echo: fenced for example. At first I did it in this PR, but then as this is some R refactoring more generic, I'll do it in another PR (#4735). We can then postpone to 1.4 if we think this is two much.

cderv added 4 commits March 9, 2023 14:06
…bed` handler

Currently the `embed` handler is only for Jupyter.

Discovered while looking at #4712 - revealjs does not work with knitr embed engine
because those engine expect echo, but revealjs default to FALSE
@cderv cderv requested a review from jjallaire March 9, 2023 13:08
@cderv cderv added the needs-discussion Issues that require a team-wide discussion before proceeding further label Mar 9, 2023
@cderv cderv removed the request for review from jjallaire March 9, 2023 17:21
@cderv
Copy link
Collaborator Author

cderv commented Mar 9, 2023

Following meeting and discussion with @cscheid this needs to be split as the embed override is in fact a more broader issue where non-cell handler should not be passed to knitr among handledLanguages

Currently all are

names(knitr::knit_engines$get())
 [1] "awk"       "bash"      "coffee"    "gawk"      "groovy"    "haskell"  
 [7] "lein"      "mysql"     "node"      "octave"    "perl"      "php"      
[13] "psql"      "Rscript"   "ruby"      "sas"       "scala"     "sed"      
[19] "sh"        "stata"     "zsh"       "asis"      "asy"       "block"    
[25] "block2"    "bslib"     "c"         "cat"       "cc"        "comment"  
[31] "css"       "ditaa"     "dot"       "embed"     "eviews"    "exec"     
[37] "fortran"   "fortran95" "go"        "highlight" "js"        "julia"    
[43] "python"    "R"         "Rcpp"      "sass"      "scss"      "sql"      
[49] "stan"      "targets"   "tikz"      "verbatim"  "ojs"       "mermaid"  
[55] "include"  

Meaning include is also pass (see the end) - embed is there as an override of the built-in one, like dot handler from Quarto which overrides the knitr one, but I think that is fine in the context of Quarto.

So we need first another PR for the handler adaptation

src/core/handlers/base.ts Show resolved Hide resolved
src/resources/rmd/hooks.R Outdated Show resolved Hide resolved
@cderv cderv requested a review from cscheid March 10, 2023 11:19
This will avoid format defaulting to echo: false to hide those chunks
Copy link
Collaborator

@cscheid cscheid left a comment

Choose a reason for hiding this comment

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

lgtm!

@cscheid cscheid merged commit e5ed82b into main Mar 10, 2023
@cderv cderv deleted the fix/reveal-knitr-verbatim branch March 10, 2023 19:45
@cderv cderv removed the needs-discussion Issues that require a team-wide discussion before proceeding further label Mar 10, 2023
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.

revealjs does not work with knitr embed engine
3 participants