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

Fixup some knitr syntax in Lua to support SQL engine output #7829

Merged
merged 4 commits into from
Dec 8, 2023

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Dec 7, 2023

This closes #4869

I believe knitr SQL engine is not correctly designed, and this creates for us some difficulty to catch its output to wrap it under output cell-output-display div. The engine sets results = 'asis' and the output is only cat() without any other processing (no hooks, or no sewing of class item). sew.character is directly applied on it and not the sew() method that we do patch.

Anyhow, i did not find any easy way to do the fixup in R without regex or else. It seemed fragile, and more robust to add a fixup stage in Lua. This is possible because a div is emitted and this is correctly parsed as pandoc.Div due to native_spans extension being the default.

Other approach would be to consider this behavior in Quarto a bug in knitr and would require an update of the .sql engine for its output to go through the proper sew() processing, or have a class we can catch (we should catch a knit_asis element but here it is missing).

Doing this in knitr would require an update of knitr to be used with Quarto - this fix here should solve for any knitr version.
This could be interesting in the meantime to have it, but if you think fixing this in knitr is better I can work on that too.

@cscheid I added the step as an init step with a conditional on engine being used. My understanding is that this should happen from the start of AST normalization before any processing as we are fixing up the input syntax. If this needs to be elsewhere, please do tell me.

We can't easily patch it at the R level so we do it at the Lua level in the first step of normalizing the AST
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.

We should change the way runPandoc sends the engine name to Lua. But everything else looks good!

src/command/render/render.ts Outdated Show resolved Hide resolved
@cderv cderv merged commit 218e03d into main Dec 8, 2023
47 checks passed
@cderv cderv deleted the knitr-eng-sql-wrapping branch December 8, 2023 11:31
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.

Article Layout Margin option for knitr sql code block seems buggy
2 participants