jacobbien / litr-project

Writing R Packages with Literate Programming
https://jacobbien.github.io/litr-project/
Other
37 stars 3 forks source link

Add hooks for messages and output to replace ANSI codes #32

Closed patrickvossler18 closed 1 year ago

patrickvossler18 commented 1 year ago

This PR addresses #26 by adding knitr hooks to replace ANSI sequences for messages and R output. I can add the updated version of the html file once #31 has been accepted. We're able to correctly print most output and messages as shown here: image

The remaining challenge is to handle the testthat output that is returned by our Rscript_call: image

Rstudio parses the testthat output which is returned in a specific format: https://testthat.r-lib.org/reference/Reporter.html . To display this output, we need to make a parser that mimics what Rstudio does here: https://github.com/rstudio/rstudio/blob/4f7063b13bb0bae8377639a36d3ff4b7626de6e3/src/cpp/session/modules/build/SessionBuildErrors.cpp#L265

jacobbien commented 1 year ago

Thanks @patrickvossler18. Now that #31 has been merged, you can create the .html. However, a bigger issue: The goal is not to change the rendering of create-litr.Rmd. Rather, the goal is to make it so that when a user uses litr, the output would be corrected. So the fix you have should actually go into the function litr::setup(). And then to test that it works, try

rmarkdown::draft("create-rhello.Rmd", template = "make-an-r-package", package = "litr")
litr::render("create-rhello.Rmd")

And then at the end of create-rhello.html, we should no longer see

## ℹ Loading rhello
## Writing ']8;;file:///Users/jacobbien/Dropbox/literate/Rpackage/litr-project/examples/rhello/NAMESPACENAMESPACE]8;;'
## Writing ']8;;ide:run:pkgload::dev_help('say_hello')say_hello.Rd]8;;'

but instead something properly rendered.

patrickvossler18 commented 1 year ago

I've merged in upstream updates and pushed a fix for pdf documents that uses an internal function from the fansi package to strip all ANSI escape codes. I chose to use their internal function because it is more robust and can better handle other ANSI escape codes we haven't yet seen in knitted documents compared with whatever regular expression I can come up with.

I also modified the litrify_output_format function to handle the case where the old output format does not have a post_processor function as is the case with the default pdf document format.

The final issue is knitting the examples with the newest version of litr based on the changes I've made to create_litr.Rmd. I know you've explained how to do this before, but I've forgotten how. I volunteer to start a development document as penance.

jacobbien commented 1 year ago

No need for penance :) but there were two things that needed adjusting as done in the commit above.

  1. For now, any function in litr needs to be assigned to one of the .R files in R/. Before next release this will be cleaned up, but leaving this system for now.
  2. For litr_pdf_document() to work it needs to return the object new.

But after making these two changes, one just runs the following (as explained here):

remotes::install_github("jacobbien/litr-project@*release", subdir = "litr")
litr::render("create-litr.Rmd")

This will re-generate all the examples.

jacobbien commented 1 year ago

Thanks @patrickvossler18 . This is looking good. Two remaining comments:

patrickvossler18 commented 1 year ago
* [ ]  The hyperlinks created by `fansi::sgr_to_html()` are broken links.  Can we fix these to go to the right places?

It looks like something is going wrong when the file path is expanded during the call to the function ide:run:pkgload::dev_help. It appears this issue is happening before we replace escape codes since the file path for NAMESPACE is correctly filled in.

One option would be to call pkgload::dev_topic_find on the function and fill in the path ourselves, but I'm not sure if this would generalize well to a setting where the knitted file is hosted online.

Another option would be to simply strip the anchor tags and just print the text. For files such as .Rd files it doesn't seem particularly useful to view the raw file anyway.

* [ ]  I believe the bookdown output format's post processor should be modified in a similar way to the html output format

I've modified the bookdown post processor to use replace_ansi_sequences and I will push the change once we've decided what we want to do about the hyperlinks

jacobbien commented 1 year ago

Thanks. Yeah, why don't we just strip the anchor tags... I agree that it doesn't seem all that useful.

patrickvossler18 commented 1 year ago

Ok, I've pushed a change that strips the escape sequences for URLs so we don't accidentally remove anchor tags that we want to keep. It looks to me like replace_ansi_sequences can be directly applied to bookdown so I added that as well. Let me know if I am mistaken.

jacobbien commented 1 year ago

Thanks @patrickvossler18. All looks good to me! I will merge this into main.