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

BUG: mark can't cope with multiple classes in class.* chunk options #106

Closed
thothal opened this issue Sep 18, 2023 · 6 comments
Closed

BUG: mark can't cope with multiple classes in class.* chunk options #106

thothal opened this issue Sep 18, 2023 · 6 comments
Assignees
Labels

Comments

@thothal
Copy link

thothal commented Sep 18, 2023

Consider the following reprex:

library(markdown)
chunk <- '```{r class.output = "op1 op2"}\n 1+1\n```\n'
cat(chunk)
# ```{r class.output = "op1 op2"}
#  1+1
# ```

cat(mark(knitr::knit(text = chunk)))
                                                                                
# <pre><code class="language-r"> 1+1
# </code></pre>
# <pre><code class="language-{.op1">## [1] 2
# </code></pre>

As you can see the rendered HTML is a mess. Changing class.output = c("op1", "op") yields the same results.

Using a single `class does work:

chunk2 <- '```{r class.output = "op1"}\n 1+1\n```\n'
cat(mark(knitr::knit(text = chunk2)))
# <pre><code class="language-r"> 1+1
# </code></pre>
# <pre><code class="language-op1">## [1] 2
# </code></pre>
xfun::session_info()
# Version 4.2.2 (2022-10-31 ucrt)
# Platform: x86_64-w64-mingw32/x64 (64-bit)
# Running under: Windows 10 x64 (build 19045)

# Locale:
#   LC_COLLATE=German_Germany.utf8  LC_CTYPE=German_Germany.utf8    LC_MONETARY=German_Germany.utf8 LC_NUMERIC=C                   
#   LC_TIME=German_Germany.utf8    

# Package version:
#   commonmark_1.9.0  compiler_4.2.2    cpp11_0.4.6       graphics_4.2.2    grDevices_4.2.2   markdown_1.8      stats_4.2.2      
#   systemfonts_1.0.4 tools_4.2.2       utils_4.2.2       xfun_0.39    
@yihui yihui added the bug label Sep 18, 2023
@yihui yihui self-assigned this Sep 18, 2023
@yihui
Copy link
Member

yihui commented Sep 18, 2023

This is because the CommonMark spec doesn't support multiple classes for code blocks (although Pandoc does):

commonmark::markdown_html('```{.op1 .op2}
## [1] 2
```')
<pre><code class="language-{.op1">## [1] 2
</code></pre>

But I think it makes sense to support this feature. I'll see what I can do.

@yihui yihui moved this from Backlog to Todo In Progress in R Markdown Team Projects Sep 19, 2023
@yihui yihui closed this as completed in 6db3be0 Sep 19, 2023
@github-project-automation github-project-automation bot moved this from Todo In Progress to Done in R Markdown Team Projects Sep 19, 2023
@yihui
Copy link
Member

yihui commented Sep 19, 2023

Should be fixed now. Thanks for the report!

You can install the development version via

remotes::install_github('rstudio/markdown')

Or wait for an hour or two and

install.packages('markdown', repos = 'https://rstudio.r-universe.dev')

Make sure packageVersion('markdown') >= '1.8.4') after installation.

@thothal
Copy link
Author

thothal commented Sep 19, 2023

Thanks for the fast fix. But there is one more thing I am buffled (I reformatted the HTML by hand for better legibility):

cc <- "```{r class.output = c(\"ss\", \"tt\")}\n1+1\n```"
cat(cc)
# ```{r class.output = c("ss", "tt")}
# 1+1
# ```

cat(knitr::knit(text = cc))
# ```r
# 1+1
# ```

# ```{.ss .tt}
# ## [1] 2
# ```

cat(mark(knitr::knit(text = cc)))

# <pre>
#   <code class="language-r">1+1</code>
# </pre>
# <pre>
#   <code class="language-ss tt">## [1] 2# </code>
# </pre>

Reading https://spec.commonmark.org/0.28/#info-string I understand that the first class element becomes prefixed with language-, but this behaviour especially in combination with several output classes is counter intuitive to say the least. So if somebody wants to provide CSS rules for code blocks like this, one has to write:

.language-ss {/* */} // must not forget to prefix my class with language-
.tt {/* */} // must not forget to NOT prefix the second class

Is it then recommended to always add a (dummy) class (r ?) as a first element as soon as one wants to style the blocks? Then at least I could write my CSS as expected:

cc <- "```{r class.output = c(\"r\", \"ss\", \"tt\")}\n1+1\n```"
cat(mark(knitr::knit(text = cc)))
# <pre><code class="language-r">1+1
# </code></pre>
# <pre><code class="language-r ss tt">## [1] 2
# </code></pre>
.ss {/* */} 
.tt {/* */}

If this is the intended behaviour, would it make sense to add an r-output (say) class by default?

@yihui
Copy link
Member

yihui commented Sep 19, 2023

I thought about this issue when implementing this feature yesterday. Since this is outside the CommonMark specs, I don't know what I should do. Adding the language- prefix to each class can be good or bad, intuitive or counterintuitive. The first class always represents the language, and the rest of classes do not necessarily. I tend not to add the language- prefix because it will make it impossible for users who do not desire this prefix to remove it. By comparison, if users do want the prefix, they have the choice of adding it by themselves (class.output = paste0('language-', ...)).

The second reason is that it should be rare that the same code in a single code block can have multiple languages (e.g., language-r language-python ...). Normally it should be for one language only.

The third reason is the rest of classes may not be language names at all, and class names like language-red language-dark language-emphasis may look odd semantically.

If this is the intended behaviour, would it make sense to add an r-output (say) class by default?

Again, I don't want to bother users who do not desire class names that they didn't specify (BTW, r-output may not be a good name for output not generated by R code). If you want a default class, it should be straightforward to add it manually, or automatically:

knitr::opts_hooks$set(class.output = function(options) {
  options$class.output = c('r-output', options$class.output)
  options
})

@thothal
Copy link
Author

thothal commented Sep 19, 2023

Adding the language- prefix to each class can be good or bad, intuitive or counterintuitive. The first class always represents the language, and the rest of classes do not necessarily.

I guess this is what I tripped over in the first place. For me, class.output="myclass" was a way to style the output and naively I thought I could do that via the .myclass selector.

After reading the specs, I understand now that the first class becomes the language class (prefixed by language-) and I should consider that in writing my CSS. Thus, my solution in teh future will be that if I want a custom class for a chunk to add both a language tag and my custom class, i.e. class.output = c("output", "myclass") and style it then with .myclass (I could use .language-output but this feels semantically wrong).

I tend not to add the language- prefix because it will make it impossible for users who do not desire this prefix to remove it. By comparison, if users do want the prefix, they have the choice of adding it by themselves (class.output = paste0('language-', ...)).

Well, they should provide the language tag (whatever this should be for an output chunk) as well because otherwise they get class="language-language-...".

The third reason is the rest of classes may not be language names at all, and class names like language-red language-dark language-emphasis may look odd semantically.

Exactly, and not knowing that the first class IS the language name (and not a custom class) created all this mess for me ;)

To sum up, as a user it was not clear to me that the first item of class.output is meant to be a language class (and thus is automatically prefixed). Maybe this misunderstanding deserves a line of clarification in one of the docs?

@yihui
Copy link
Member

yihui commented Sep 19, 2023

To sum up, as a user it was not clear to me that the first item of class.output is meant to be a language class (and thus is automatically prefixed). Maybe this misunderstanding deserves a line of clarification in one of the docs?

Good suggestion. I'll do it. Thanks!

clrpackages pushed a commit to clearlinux-pkgs/R-markdown that referenced this issue Oct 3, 2023
Yihui Xie (13):
      start the next version
      finally I can get rid of this internal function thanks to the leaflet release v2.2.0: https://github.com/rstudio/leaflet/releases/tag/v2.2.0
      use PCRE to fix #104, in the same way as a5b42c2
      define an internal version of gregexpr() that has perl = TRUE by default
      add a comment so that I can remember why I'm using perl = FALSE here
      gregexpr() has perl = TRUE by default now, so no need to have this extra argument (`...` is enough)
      rmarkdown 2.18 was released on 2022-11-09; I assume it's old enough that not many people still use lower versions
      close #105: support HTML widgets
      fix #106: support attributes for fenced code blocks
      add documentation for fenced code blocks: rstudio/markdown#106 (comment)
      check the availability of rmarkdown before processing HTML dependencies
      return early if there are no HTML dependencies to process
      CRAN release v1.9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

No branches or pull requests

2 participants