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

Allow the knit button to work in subdirs #1122

Merged
merged 7 commits into from
Apr 2, 2021
Merged

Allow the knit button to work in subdirs #1122

merged 7 commits into from
Apr 2, 2021

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Apr 1, 2021

fixes #1121

It is like with blogdown, with this adjustment a Rmd file in a subdir can be previewed using the knit button.

Only issue is that RStudio IDE won't open the preview because

  • rmarkdown::render() returns a relative path e.g _book/02-litterature.html to the bookdown project
  • RStudio IDE consider a relative path as being relative to the input file - which in this case is not because the Rmd file is in a sub directory.

Rmarkdown website deals with that by suppressing message for render() to avoid Output created: to be written by render in console and throw itself the message with render_site() - this happens in default_site().

I tried adding one message in render= inside bookdown_site() but as render() already emits one, only the first is taken into account it seems.

We could do the same in bookdown but it seems a bit too much to suppress message from render() completly.
What about a new option to introduce in rmarkdown that would allow us to deactivate the printing of this message in render() from other package ?

@yihui what do you think we could do ?

I put as draft so that we decide what to do, but it seems making the Knit button work for a bookdown project is important. I recognize that using subdirs and preview may not be a lot of cases.

Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

What about a new option to introduce in rmarkdown that would allow us to deactivate the printing of this message in render() from other package ?

I have wished to have a way to suppress only the "Output created" message in the past, too, but I hesitated to add one more argument to rmarkdown::render() just for this purpose. I just thought about it again, and perhaps we could use withCallingHandlers() to capture the message, suppress it, and signal it later. Here is an example:

writeLines(c('---', 'title: asdf', '---', '', 'Test'), 'test.Rmd')

msg_render = NULL
withCallingHandlers(rmarkdown::render('test.Rmd'), message = function(e) {
  if (grepl('^\nOutput created: ', e$message)) {
    # capture and save the message here, then muffle it
    msg_render <<- e
    invokeRestart("muffleMessage")
  }
})
if (!is.null(msg_render)) message(msg_render)

Of course, you can modify msg_render before actually emitting the message (e.g., you can tweak the file path contained in the message if necessary).

Does this approach sound okay to you?

@cderv
Copy link
Collaborator Author

cderv commented Apr 2, 2021

I have wished to have a way to suppress only the "Output created" message in the past, too, but I hesitated to add one more argument to rmarkdown::render() just for this purpose.

By "options", I meant an R option not a new argument to render() - something like options(rmarkdown.rstudio.preview = FALSE) that we would look for before emitting the message or not. It seems easy to set this option before calling rmarkdown::render() from another package. It is one more option though...

I did not though about handling the message. That is indeed a very good way of doing it! I'll try that. Thank you !

@cderv cderv marked this pull request as ready for review April 2, 2021 12:48
@cderv
Copy link
Collaborator Author

cderv commented Apr 2, 2021

@yihui this is now working as expected.

Thanks for your idea - it made completely sense and reminded me of suppressMessage.

As this is more advanced, I am just letting you check the implementation before merging. Hope that is ok!

@cderv cderv requested a review from yihui April 2, 2021 12:49
Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

By "options", I meant an R option not a new argument to render() - something like options(rmarkdown.rstudio.preview = FALSE) that we would look for before emitting the message or not. It seems easy to set this option before calling rmarkdown::render() from another package. It is one more option though...

I see. I'm okay with adding a global option. That seems to be cleaner than the withCallingHandlers() method, so I'd probably do that instead. As you mentioned, it will also allow other packages to suppress the message. We've run into this problem not only in bookdown.

@cderv
Copy link
Collaborator Author

cderv commented Apr 2, 2021

ok. I suggest we merge this as-is because it is working now and it won't require a new rmarkdown version yet for bookdown.

I'll do the change in rmarkdown and we'll change it here when rmarkdown will be on CRAN.
Does that sound good ?

@yihui
Copy link
Member

yihui commented Apr 2, 2021

Yes.

@yihui yihui merged commit 36f4dca into master Apr 2, 2021
@yihui yihui deleted the knit-button-subdir branch April 2, 2021 14:14
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

preview_chapter() does not work with Rmd files in a sub-directory
2 participants