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

tidy=TRUE option not working in before_chapter_script when new_session=TRUE (knit and merge method) #1049

Closed
3 tasks
turtlegraphics opened this issue Dec 22, 2020 · 9 comments
Labels
question general questions - not an issue

Comments

@turtlegraphics
Copy link

I'd like to automatically tidy all R code in the book I'm working on. The book already has a before_chapter_script, so I want to add this line to it:

knitr::opts_chunk$set(tidy=TRUE)

It seems to not be having an effect.

I do see tidying working when I add tidy=TRUE to a chunk as an option. It also works if I include the line above as the first chunk in the file.

I also tried changing tidy=TRUE to echo=FALSE in the before_chapter_script to see if it would do anything, and it does turn off all echoing.

I can't imagine why this particular setting won't work while others do, so I'm posting here.


By filing an issue to this repo, I promise that

  • I have fully read the issue guide at https://yihui.org/issue/.
  • I have provided the necessary information about my issue.
    • If I'm asking a question, I have already asked it on Stack Overflow or RStudio Community, waited for at least 24 hours, and included a link to my question there.
    • If I'm filing a bug report, I have included a minimal, self-contained, and reproducible example, and have also included xfun::session_info('bookdown'). I have upgraded all my packages to their latest versions (e.g., R, RStudio, and R packages), and also tried the development version: remotes::install_github('rstudio/bookdown').
    • If I have posted the same issue elsewhere, I have also mentioned it in this issue.
  • I have learned the Github Markdown syntax, and formatted my issue correctly.

I understand that my issue may be closed if I don't fulfill my promises.

@cderv
Copy link
Collaborator

cderv commented Dec 23, 2020

Hi,

What tells you it is not working ? It works as expected when I tried.

You did not provide a reproducible example so I needed to build one.

I tried with bookdown-demo (https://github.com/rstudio/bookdown-demo) :

  1. Clone the project
  2. Create a script _common.R with this content knitr::opts_chunk$set(tidy=TRUE)
  3. Add before_chapter_script: _common.R in bookdown.yml
  4. Add this chunk in any doc, for example 02.literature.Rmd
    ```{r}
    mean(
      1    +   2 
    )
    ```
  5. Build the book

You will have a chunk correctly formated by formatR, resulting of tidy = TRUE in the resulting HTML

mean(1 + 2)

Remove the before_chapter_script in bookdown.yml and you'll have a chunk with the formatting unchange

mean(
  1    +   2 
)

So this is working fine.

If it is not working for you, please follow the issue guide: Having a reproducible example will insure we can understand the problem bette and have all the required information: there is many configuration you can set in a bookdown project!

@cderv cderv closed this as completed Dec 23, 2020
@cderv cderv added the question general questions - not an issue label Dec 23, 2020
@cderv cderv reopened this Dec 23, 2020
@cderv
Copy link
Collaborator

cderv commented Dec 23, 2020

Just a confirmation.

Do you have in your bookdown.yml ?

new_session: true

I think I can reproduce with this option set to TRUE. (A reproducible example would have been easier to follow here to find how you encountered the issue)

@cderv
Copy link
Collaborator

cderv commented Dec 23, 2020

I can confirm there is a bug with Knit and Merge approach (new_session: true) since the change in 581bb63

We execute the before_chapter_script before calling rmarkdown::render. So it correctly sets the option tidy = TRUE but rmarkdown::render will set this option to FALSE by default (https://github.com/rstudio/rmarkdown/blob/5c0aeb078cebb077967e5ee18bfebb937e942d4e/R/render.R#L607)

It is why it works with echo and not with tidy as this former is not modified by render().

Before the change in 581bb63, the before_chapter_script was inserted inside the Rmd document, so it would correctly override any default sets by rmarkdown::render(). But we made the change because it would mess with line numbering when errors with knitr

This before_chapter_script and after_chapter_script are always tricky.

You will encounter this issue only with Knit and Merge approach. With new_session: false, which is the default, it will work.

One way to insert common elements in books chapter is to source the script for a chunk in your document directly, or use a child Rmd file.

Several ways among them these 2

  • Use a child document (e.g. _common.Rmd) with this content

    ```{r, include = FALSE, echo = FALSE}
    knitr::opts_chunk$set(tidy = TRUE)
    ```
    

    and add

    ```{r child = "_common.Rmd"}
    ```
    
  • Source a script in a chunk

    ```{r include = FALSE}
    source("_common.R")
    ```

We'll think if this is something we can fix. This issue will only be encountered with option that rmarkdown::render set as default and I think it is mainly tidy = FALSE and error = FALSE that a user would try to change. The others are more oriented toward internal use.

@yihui did you have this side effect in mind when inserting the change in 581bb63 ? Is there any workaround we could find to also avoid this while keeping 581bb63 ?

Otherwise, maybe we should advice to use child Rmd or sourcing R script in each docs as it is efficient and with less side effect as it is user controlled and not a feature we try to maintain. 😅

@cderv cderv added bug an unexpected problem or unintended behavior and removed question general questions - not an issue labels Dec 23, 2020
@yihui
Copy link
Member

yihui commented Dec 23, 2020

Sorry I didn't foresee this problem. Another workaround is to use options(knitr.chunk.tidy = TRUE) in the before_chapter_script. This is an undocumented way to set global chunk options, although it has existed for several years.

Otherwise, maybe we should advice to use child Rmd or sourcing R script in each docs as it is efficient and with less side effect as it is user controlled and not a feature we try to maintain.

This seems to be a corner case that most users probably won't run into (tidy and error are the only two chunks options of which the default values are modified by rmarkdown), and the workaround is relatively simple, so I wouldn't recommend using child Rmd or sourcing R scripts in each Rmd.

@cderv
Copy link
Collaborator

cderv commented Dec 23, 2020

This seems to be a corner case that most users probably won't run into (tidy and error are the only two chunks options of which the default values are modified by rmarkdown), and the workaround is relatively simple, so I wouldn't recommend using child Rmd or sourcing R scripts in each Rmd.

This confirms what I thought. Thank you !
I'll leave this opened in case anyone else wonder about this.

@cderv cderv added question general questions - not an issue and removed bug an unexpected problem or unintended behavior labels Dec 23, 2020
@cderv cderv changed the title tidy=TRUE option not working in before_chapter_script tidy=TRUE option not working in before_chapter_script when new_session=TRUE (knit and merge method) Dec 23, 2020
@turtlegraphics
Copy link
Author

Apologies for not providing a MWE. I really appreciate the help and the workaround.

@turtlegraphics
Copy link
Author

turtlegraphics commented Dec 23, 2020

I believe this same issue is affecting knitr::knit_hooks$set(error = ...), since that also works for me if I put it in the .Rmd source but not if I put it in the before_chapter_script.

local({
  hook_old <- knitr::knit_hooks$get("error")  # save the old hook
  error_handler_function <- function(x, options) {
    # wrap x
    x <- paste(strwrap(x,76), collapse="\n## ")
    x <- paste(x,'\n',sep='')
    # pass the new x to the old hook
    hook_old(x, options)
  }
  knitr::knit_hooks$set(error = error_handler_function)
})

Is there an options workaround here as well?

@cderv
Copy link
Collaborator

cderv commented Mar 31, 2021

Are you using the last version of bookdown ?

This is working for me when I had this in dummy.R

local({
  hook_old <- knitr::knit_hooks$get("error")  # save the old hook
  error_handler_function <- function(x, options) {
    # wrap x
    x <- paste(strwrap(x,76), collapse="\n## ")
    x <- paste(x,'\n',sep='')
    # pass the new x to the old hook
    hook_old(x, options)
  }
  knitr::knit_hooks$set(error = error_handler_function)
})

and this in _bookdown.yml

before_chapter_script: dummy.R

and this is 01-intro.Rmd

```{r, error = TRUE}
# to be wrap by the hook
err <- sample(c("a", " "), size = 100, replace = TRUE, prob = c(0.8,0.2))
stop(paste0(err, collapse = ""))
```

All this is not really issues with bookdown. If you still have questions, please ask on one of the Q&A site: https://yihui.org/issue/#got-a-question

If you found a new issue, open a new one with a specific example to that issue. Having one topic per issue in Github is way easier for us.

Thank you !

@github-actions
Copy link

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue by following the issue guide (https://yihui.org/issue/), and link to this old issue if necessary.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question general questions - not an issue
Projects
None yet
Development

No branches or pull requests

3 participants