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

Correctly handles new knitr 1.44 option normalization #7084

Merged
merged 4 commits into from
Sep 29, 2023

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Sep 29, 2023

Change was introduced in knitr to fix quarto issues #5852 and #6576.

Pre 1.44, knitr was only normalizing fig- to fig. and out- to out..
Post 1.44, knitr is normalizing from - version to . version its own known knitr chunk options.

This means we need to now carefully handle backward compatibility to work with all versions.
For future, this simplifies because Quarto-specific options will only be using - versions even during knitting.

This contribute to solve issue #7078 as fig-column will be correctly forwarded as a class to the output div.
However, Quarto 1.4 breaks layout when fig-column-* is present.

@cscheid how do you suggest we add some tests for the expected Markdown output so that we can prevent any breakage in knitr options handling ?

Change was introduced in knitr to fix quarto issues #5852 and #6576.

Pre 1.44, knitr was only normalizing `fig-` to `fig.` and `out-` to `out.`.
Post 1.44, knitr is normalizing from - version to . version its own known knitr chunk options.

This means we need to now carefully handle backward compatibility to work with all versions.
For future, this simplifies because Quarto-specific options will only be using - versions even during knitting.
…e - and . correctly

- add missing hidden options created by Quarto
- separate quarto opts identical to knitr ones for clarity
This is required for knitr < 1.44 compatibility
as since 1.44 these options are aliased in knitr itself
respectively to dev and dpi options.
@cscheid
Copy link
Collaborator

cscheid commented Sep 29, 2023

That's a good question, but I think it will require a slower answer. I think we should merge this, and consider those tests in the separate issue we opened.

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.

(With low confidence)

This looks good to me.

Small concern: when there's a big list of string constants like, it can be helpful to write in a comment where they came from, so that we can keep that information close to where it matters.

@cscheid cscheid merged commit c261dfc into main Sep 29, 2023
47 checks passed
@cscheid cscheid deleted the fix/knitr-options-normalization branch September 29, 2023 20:10
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.

knitr 1.44 breaks Quarto's figure layout classes
2 participants