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 replace knitr options which had their name normalized from - to . #5852

Closed
cderv opened this issue Jun 8, 2023 · 4 comments · Fixed by yihui/knitr#2282
Closed
Assignees
Labels
bug Something isn't working knitr
Milestone

Comments

@cderv
Copy link
Collaborator

cderv commented Jun 8, 2023

Example from

This below won't work

---
format: html
---

```{r}
#| tidy: true
#| tidy-opts: !expr list(width.cutoff = 50)
# a long expression
1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+
1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1
```

This is because

  • knitr defines tidy.opts = NULL as a default
  • knitr will parse the YAML chunk option and sees tidy-opts so it won't replace the default. Options have both now tidy-opts and tidy.opts
  • Quarto will normalize - to . in an option hook step, but this will just rename. This means options in knitr have twice tidy.opts with two different value.
  • Knitr will use only the first value, which will be the default one.

Our options hooks should make the replacement.

Longer term, knitr should probably do the normalization as the YAML chunk option parsing step

opts_hooks[["code"]] <- function(options) {
lastYamlCode <<- options[["yaml.code"]]
options <- knitr_options_hook(options)
if (is.null(lastYamlCode)) {
lastYamlCode <<- options[["yaml.code"]]
}
options
}

} else {
# convert any option with fig- into fig. and out- to out.
options <- normalize_options(options)
}

@cderv cderv added bug Something isn't working knitr labels Jun 8, 2023
@cderv cderv added this to the v1.4 milestone Jun 8, 2023
@cderv cderv self-assigned this Jun 8, 2023
@cderv
Copy link
Collaborator Author

cderv commented Aug 24, 2023

@yihui I would like your thoughts on this.

Currently in knitr we do not normalize all the dashed form option like tidy-opts or cache-lazy. Quarto does it currently but after knitr parsing.

} else {
# convert any option with fig- into fig. and out- to out.
options <- normalize_options(options)
}

normalize_options <- function(options) {
names(options) <- sapply(names(options), function(name) {
if (name %in% c(
# Text output
"strip-white",
"class-output",
"class-message",
"class-warning",
"class-error",
"attr-output",
"attr-message",
"attr-warning",
"attr-error",
# Paged tables
"max-print",
"sql-max-print",
"paged-print",
"rows-print",
"cols-print",
"cols-min-print",
"pages-print",
"paged-print",
"rownames-print",
# Code decoration
"tidy-opts",
"class-source",
"attr-source",
# Cache
"cache-path",
"cache-vars",
"cache-globals",
"cache-lazy",
"cache-comments",
"cache-rebuild",
# Plots
"fig-path",
"fig-keep",
"fig-show",
"dev-args",
"fig-ext",
"fig-width",
"fig-height",
"fig-asp",
"fig-dim",
"out-width",
"out-height",
"out-extra",
"fig-retina",
"resize-width",
"resize-height",
"fig-align",
"fig-link",
"fig-env",
"fig-cap",
"fig-alt",
"fig-scap",
"fig-lp",
"fig-pos",
"fig-subcap",
"fig-ncol",
"fig-sep",
"fig-process",
"fig-showtext",
# Animation
"animation-hook",
"ffmpeg-bitrate",
"ffmpeg-format",
# Code chunk
"ref-label",
# Language engines
"engine-path",
"engine-opts",
"opts-label",
# Other chunk options
"R-options")) {
sub("-", ".", name)
} else {
name
}
}, USE.NAMES = FALSE)
options
}

However, it is missing the merging step, meaning that if cache.lazy exist already in options (and it does because it has a default value), then renaming options$cache-lazy to options$cache.lazy just creates a duplicate.

Initially it was done by Quarto, but now this step is run only for old knitr

if (!knitr_has_yaml_chunk_options()) {
# partition yaml options
results <- partition_yaml_options(options$engine, options$code)
if (!is.null(results$yaml)) {
# convert any option with fig- into fig. and out- to out.
# we need to do this to the yaml options prior to merging
# so that the correctly interact with standard fig. and
# out. options provided within knitr
results$yaml <- normalize_options(results$yaml)
# alias 'warning' explicitly set here to 'message'
if (!is.null(results$yaml[["warning"]])) {
options[["message"]] = results$yaml[["warning"]]
}
# merge with other options
options <- knitr:::merge_list(options, results$yaml)
# set code
options$code <- results$code
}
options[["yaml.code"]] <- results$yamlSource
# some aliases
if (!is.null(options[["fig.format"]])) {
options[["dev"]] <- options[["fig.format"]]
}
if (!is.null(options[["fig.dpi"]])) {
options[["dpi"]] <- options[["fig.dpi"]]
}
} else {

In yihui/knitr@e24dcb4, only normalization for fig- and out- was added. But in fact, all dashed-version of knitr options should be transformed to the dot version.

If we do that in knitr directly, it would solve issue in Quarto.

Related issues is

Should we do that in knitr ?

Otherwise, I need to tweak options after knitr parsing to fix this, or maybe revert back to quarto parsing the YAML options using new exported knitr::partition_chunk() directly.

Thanks for your input.

@yihui
Copy link
Contributor

yihui commented Aug 24, 2023

I think we can normalize all option names inside knitr. I'll take a closer look today.

@cderv
Copy link
Collaborator Author

cderv commented Aug 24, 2023

Ok great ! Here is the branch I had in case we wanted to fix both issues in Quarto
main...fix/knitr/option-normalization

It seems better to me to do it in knitr, even if this means that user's would have to update knitr.

@cderv
Copy link
Collaborator Author

cderv commented Aug 28, 2023

This has been fixed on knitr - from next version 1.44 (and from current dev version 1.43.7), all dash option are changed to their dot version. This includes tidy-opts and cache-lazy

cderv added a commit that referenced this issue 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working knitr
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants