Convert dashes in chunk option names to dots

[yihui]

Fix quarto-dev/quarto-cli#5852.

@cderv Instead of enumerating specific options, I'm just substituting all dashes to dots. The advantage is that we don't need to maintain a list of hard-coded option names (e.g., if we add new chunk options to knitr in future, we won't need to update this list). The downside is that users might really want to use dashes in option names, but I think this should be rare.

Also fix quarto-dev/quarto-cli#6576.

[cderv]

Oh so you do that in general fix_options 🤔

I was thinking to keep that YAML parsing specific, in the partition function.

Like

• Parsing YAML options - those could be dashed version probably
• Substitute - by .
• Use knitr::merge_list() to merge so options parsed from YAML with any other existing, including the knitr default one.

Doing in fix_options() is more general. Could be ok if you think so.
Only thoughs are:

• Indeed, if someone is already using knitr option with -, it will break (probably rare as you think)
• I did the equivalent of that in my branch on Quarto side

  dashes = grep('-', names(options), value = TRUE)
  options[gsub('-', '.', dashes)] = options[dashes]
  options[dashes] = NULL

and I though this could may have side effect considering order if some . options are after their - version in options - Something related to matter of order when duplicated and which should be taken first.

But I guess the situation may not happen.

Other than those thoughts, it looks good to me.

[yihui]

I was thinking to keep that YAML parsing specific, in the partition function.

The problem with that is users might have set global chunk options (which contain dashes) in the YAML metadata of the Quarto document. By doing the conversion in fix_options(), we don't need to worry where the options come from (global or local, comma-separated or YAML, etc.).

and I though this could may have side effect considering order if some . options are after their - version in options - Something related to matter of order when duplicated and which should be taken first.

You fixed this problem in quarto-dev/quarto-cli@0af9c8a. I'm using the same fix. Previous in Quarto the list of chunk options could contain duplicated options like list(tidy.opts = ..., tidy.opts = ...) (with the latter tidy.opts converted from tidy-opts). With your fix, this problem shouldn't occur again.

I'll let you merge this PR since I don't have the privilege to close issues in the quarto-cli repo. If you merge it, it may automatically close the two issues. Thanks!

[eitsupi]

Maybe closing #2214?

[cderv]

@yihui this PR is breaking Quarto right now.

I think this all lies in your original comment

Instead of enumerating specific options, I'm just substituting all dashes to dots. The advantage is that we don't need to maintain a list of hard-coded option names (e.g., if we add new chunk options to knitr in future, we won't need to update this list). The downside is that users might really want to use dashes in option names, but I think this should be rare.

Quarto is only normalizing known knitr options currently. It means it assumes that unknown knitr option will still have dash (-) in their name.

https://github.com/quarto-dev/quarto-cli/blob/33c9f8557c0afd35085980cc6560eddd69ca967c/src/resources/rmd/hooks.R#L270-L284

I am trying to see if this is only something of hard coded dashed version in R code to be used with dot version, but I believe this is more generic.

I think Quarto was assuming that unknown knitr options would still have their dash in options within knitr scope, and only known knitr options would be normalized to their dot version.

For example here, layout-ncol is supposed to be an option found in options and used as-is (dashed version) to create a Divs attributes: https://github.com/quarto-dev/quarto-cli/blob/33c9f8557c0afd35085980cc6560eddd69ca967c/src/resources/rmd/hooks.R#L253-L267

We should discuss this IMO.

• Either we think this is better to normalize everything, and we should make the change in knitr with same time update in Quarto
• Either we need to do the same as in Quarto and normalize only known knitr options

Overall, there is a scope question IMO:

• What knitr should do on its side ?
• What Quarto should on its side ?
• How that change from current situation ?

It seems that either on knitr or quarto side there will be a list of options to maintain. (we could ease that by having an object for this like knitr:::opts_chunk_attr

[cderv]

And also fix_options() happens in knitr after opts_hooks so we need to adapt Quarto code also for this PR to work because previous normalization happened in parser for fig- and out- and this is later now. (Quarto does its normalizaton in opts_hooks[["code"]] also). This creates issue with fig-cap for example now.

We may need to revert this PR and synchronize it with next Quarto version to not create breakage - we have the conflict Quarto version + knitr version to deal with 🤔

[yihui]

Okay, I'm glad that we started testing Quarto against the dev version of knitr. First, I think we should definitely normalize the dashes before opts_hooks runs. Second, we need to limit the option names to be normalized: probably only those in unique(c(names(knitr:::opts_chunk_attr), names(knitr::opts_chunk$get()))) (if any other options need to be normalized, we can add them to opts_chunk_attr since this object can be freely changed but not opts_chunk).

[yihui]

I just pushed another commit, and I think the problems should be mitigated. Let's see if there is anything else we missed.

[cderv]

It seems to be all good on Quarto side now - at least for everything that is testing in CI.

Two things to do with this new commit:

• Remove / modify the unit test I added in this initial PR
• Adapt or fix the knitr-example one

I guess you are already onto this.

[yihui]

Yes, I think I'm done now. Waiting for CI jobs to finish.