-
Notifications
You must be signed in to change notification settings - Fork 335
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
support \Sexpr[results=verbatim] #2510
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
I think you can ignore the build failures; they look like they should go away by themselves.
tests/testthat/test-rd-html.R
Outdated
@@ -159,6 +159,10 @@ test_that("can control \\Sexpr output", { | |||
expect_equal(rd2html("\\Sexpr[results=hide]{1}"), character()) | |||
expect_equal(rd2html("\\Sexpr[results=text]{1}"), "1") | |||
expect_equal(rd2html("\\Sexpr[results=rd]{\"\\\\\\emph{x}\"}"), "<em>x</em>") | |||
expect_equal(rd2html("\\Sexpr[results=verbatim]{1 + 2}"), | |||
c("<pre>", "[1] 3", "</pre>")) | |||
expect_equal(rd2html("\\Sexpr[results=verbatim]{cat(42)}"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please include a test for mingling implicitly output and side effects?
And can you please indent following https://style.tidyverse.org/syntax.html#long-lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming you meant something like {cat('42!\n'); 3}
, I added that in the amended commit.
I forgot to say: Please feel free to amend this patch/branch as needed.
R/rd-html.R
Outdated
@@ -204,13 +204,21 @@ as_html.tag_Sexpr <- function(x, ...) { | |||
on.exit(setwd(old_wd), add = TRUE) | |||
|
|||
# Environment shared across a file | |||
res <- eval(parse(text = code), context_get("sexpr_env")) | |||
outlines <- utils::capture.output({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the capturing is only needed for verbatim, I think it would be slightly clearer to compute results
earlier and then handle verbatim in it's own early-returning if
block. Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Done in the amended commit.
R/rd-html.R
Outdated
|
||
results <- options$results %||% "rd" | ||
switch(results, | ||
text = as.character(res), | ||
rd = flatten_text(rd_text(as.character(res))), | ||
hide = "", | ||
verbatim = paste0("<pre>\n", | ||
paste0(escape_html(outlines), collapse = "\n"), | ||
"\n</pre>\n"), | ||
cli::cli_abort( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be worth updating this error message since we now support all known forms of \Sexpr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the error message as suggested. Note, however, that it is currently unreachable: other values not (partially) matching one of the 4 types would already fail from parse_Rd()
.
2fcabe5
to
83e6c82
Compare
Thank you! |
pkgdown does not yet support Rd
\Sexpr
code whose output is meant to be included verbatim:I have been patching my local pkgdown installation for a while to support such output and now decided to try a PR. This made me spot a very similar PR from almost 5 years ago (#1053 by @jeroen), which was withdrawn at the time.
Could this be revisited?