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

Refactor chunk hook to handle verbatim-like engine better #4735

Merged
merged 7 commits into from
Sep 6, 2023

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Mar 9, 2023

WIP: needs #4732 to be merged before this is then merged in main. The current diff is compared the branch in #4732

This is a draft PR and should not be merged yet. Opened for discussion and sharing work.

verbatim and embed knitr's engine should not be exception and their lang option needs to be used correctly.

  • refactor handling of block attributes
  • create backticks enclosing dynamically according to code content. This is to correctly handle potential block code content using backticks already like fenced.echo in Quarto or verbatim-like engine in knitr to show markdown content.
  • Add more comments to remember why and how it works
  • exclude some engine from chunk hook .cell-code related processing

Probably require some specific review to decide if this is worth doing in 1.3 or later.

Warning: if we don't want to move knitr min requirement to 1.38, we need to copy create_fence() for its logic into our R code.

@cderv cderv added this to the v1.4 milestone Mar 9, 2023
@cderv cderv added the needs-discussion Issues that require a team-wide discussion before proceeding further label Mar 9, 2023
Base automatically changed from fix/reveal-knitr-verbatim to main March 10, 2023 19:45
@eitsupi
Copy link
Contributor

eitsupi commented Mar 14, 2023

@cderv Hi, I tried Quarto 1.3 today and noticed that prql code blocks by prqlr is not rendering as expected. (PRQL/prqlc-r#109)
I also noticed that the lang option is ignored.

I am planning a new release of prqlr, do I need to switch here to use the engine option instead of the lang option to change the language of the rendered code blocks?

@cderv cderv force-pushed the knitr/verbatim-engine-improvment branch from daf0081 to 5f88417 Compare March 14, 2023 13:49
@cderv
Copy link
Collaborator Author

cderv commented Mar 14, 2023

Thanks for reaching out @eitsupi.

Yes quarto is not perfect for custom engine yet. lang option support is part of this PR on hold.

lang <- tolower(options$lang %||% knitr:::eng2lang(options$engine))

This is currently missing in 1.3 where we have

lang <- tolower(options$engine)

This is not the only issue hence this refactoring PR.

We are "freezing" changes for now as we are preparing for 1.3 release - that is why this PR is on hold. However, I'll see with the team if I can cherry-pick this change into main before 1.3 release.

Otherwise, you'll need some workaround in your custom engine yes. Using options$engine will be one for now.

I wonder if there are other issues with custom engine like yours and maybe we need special attributes in knitr so that Quarto can know which is the type of engine and how it should behave.

I'll try your engine and report on your repo.

@eitsupi
Copy link
Contributor

eitsupi commented Mar 14, 2023

@cderv Thank you for your immediate reply!
I also tried the glue sql engine by the glue package and that one worked fine. (glue sql uses the engine option to convert output to sql)

Another problem I found was that the code annotations did not work for prql code block (Should be related to #4823).

eitsupi added a commit to PRQL/prqlc-r that referenced this pull request Mar 14, 2023
@allenmanning allenmanning removed the needs-discussion Issues that require a team-wide discussion before proceeding further label Mar 29, 2023
@cderv
Copy link
Collaborator Author

cderv commented Apr 11, 2023

  • To check with this PR:
    This
```{r}
#| output: asis
#| echo: false
file <- readLines("assets/why-shiny.shinylive")
cat(paste0(file, collapse = "\n"))
``` 

should be possible to write

```{asis}
#| file: why-shiny.shinylive
``` 

I think it needs special handling in Quarto.

…ang option needs to be used

- refactor handling of block attributes
- create backticks enclosing dynamically according to code content. This is to correctly handle potential block code content using backticks already like `fenced.echo` in Quarto or `verbatim`-like engine in knitr to show markdown content.
- Add more comments to remember why and how it works
- exclude some engine from chunk hook `.cell-code` related processing
@cderv cderv force-pushed the knitr/verbatim-engine-improvment branch from 5f88417 to dab8235 Compare September 5, 2023 16:46
@cderv cderv marked this pull request as ready for review September 6, 2023 11:26
@cderv cderv merged commit 9e012de into main Sep 6, 2023
@cderv cderv deleted the knitr/verbatim-engine-improvment branch September 6, 2023 11:27
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.

3 participants