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

prevent to_md() from clobbering document (and friends!) #15

Merged
merged 5 commits into from
May 29, 2020

Conversation

zkamvar
Copy link
Member

@zkamvar zkamvar commented May 28, 2020

Description

I've done a couple of things here (I apologize for shoving too many things in a single PR, but I figured they were all roughly related):

  1. Fixed a bug I introduced in add sourcepos argument #13/ensure Rmd chunks retain sourcepos attribute #14 where code chunks would have an extra sourcepos option added if read in with sourcepos = TRUE (whoops 😳)
  2. Updated to_xml() to only post-process code blocks that have a language attribute to allow bare or unevaluated code fences.
  3. I noticed that the original xml document was being affected by the round trip (because xml2 objects operate on pass-by-reference, which is summarized SO WELL in this coffee cup gif), so I have updated the processing to copy the document before processing and it no longer goes through a disk write (there was a weird namespace issue with this process that was affecting the block processing, so I use the technique I found in my own package)

Example

library(tinkr)
library(magrittr)
path <- system.file("extdata", "example2.Rmd", package = "tinkr")
rmd <- tinkr::to_xml(path, sourcepos = TRUE)
rmd$body %>%
  xml2::xml_find_first(".//d1:code_block") %>%
  xml2::xml_attrs()
#>  sourcepos      space   language       name    include       eval 
#>  "2:1-4:3" "preserve"        "r"    "setup"    "FALSE"     "TRUE"
tmp <- tempfile()
to_md(rmd, tmp)
# blocks are processed correctly
readLines(tmp) %>%
  head(10) %>%
  cat(sep = "\n")
#> ---
#> title: "Untitled"
#> author: "M. Salmon"
#> date: "September 6, 2018"
#> output: html_document
#> ---
#> 
#> ```{r setup, include=FALSE, eval=TRUE}
#> knitr::opts_chunk$set(echo = TRUE)
#> ```
# unevaluated blocks are processed correctly
readLines(tmp) %>%
  tail(29) %>%
  head(10) %>%
  cat(sep = "\n")
#> 
#> Non-RMarkdown blocks are also considered
#> 
#> ```bash
#> echo "this is an unevaluted bash block"
#> ```
#> 
#> ```
#> This is an ambiguous code block
#> ```
# The attributes have not changed
rmd$body %>%
  xml2::xml_find_first(".//d1:code_block") %>%
  xml2::xml_attrs()
#>  sourcepos      space   language       name    include       eval 
#>  "2:1-4:3" "preserve"        "r"    "setup"    "FALSE"     "TRUE"

Created on 2020-05-28 by the reprex package (v0.3.0)

This serves as a followup to ropensci#14
 - to_xml only processes info of blocks that have curly braces
 - transform_code_blocks only looks for the code blocks with a language  attribute (avoids ```{NA} code chunks)
 - example blocks have been added to Rmarkdown example
 - tests incorporated
The to_md() function would clobber the original xml document because all of the functions on xml documents are pass by reference instead of the trusty ol' R pass-by-value :/
@zkamvar zkamvar changed the title Znk code processing prevent to_md() from clobbering document (and friends!) May 28, 2020
Copy link
Member

@maelle maelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a ton!!

#' # file.edit("newmd.md")
#' file.remove(newmd)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much better! should I submit this to CRAN one day? If I do you'll have to become an author.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have on CRAN at some point, but I might like to give #9 a shot first 😉

code_blocks <- xml %>%
xml2::xml_find_all(xpath = './/d1:code_block',
xml2::xml_find_all(xpath = './/d1:code_block[@language]',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so much more elegant! ✨

@maelle
Copy link
Member

maelle commented May 29, 2020

Would it be fine to add you as an author before merging? If so, please go ahead and add your metadata.

@maelle maelle merged commit 2acfbea into ropensci:master May 29, 2020
@zkamvar zkamvar deleted the znk-code-processing branch September 22, 2020 22:31
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.

None yet

2 participants