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

add linebreaks before and after figures #2033

Closed
wants to merge 3 commits into from

Conversation

atusy
Copy link
Collaborator

@atusy atusy commented Aug 5, 2021

to solve #2032

@atusy
Copy link
Collaborator Author

atusy commented Aug 5, 2021

I think we need 2 linebreaks before and after the plots so that they become individual blocks.
But this certainly breaks the test below.
Is it okay to update the test? @yihui

Error: (img_output("a.png") %==% "![](a.png)") is not TRUE

https://github.com/yihui/knitr/pull/2033/checks?check_run_id=3251862194#step:11:137

@yihui
Copy link
Owner

yihui commented Aug 9, 2021

A breaking change is sometimes okay. I'm totally fine with breaking tests in this package. The much trickier and harder problem is to deal with is when the change breaks tests in other people's packages, over which I don't have control.

We can try and see how bad the breaking change is. Thanks!

@cderv
Copy link
Collaborator

cderv commented Aug 16, 2021

I think this could break some outputs if this is no more possible to generate inline plots. The HTML output will be different (using inline img vs using figure environment) and this could break some specific styling maybe. As often with rmarkdown changes, I don't think this will concern R developer that have tests, but more users with their outputs.

Also, I am trying to understand why we are finding this now. It feels to me like there is a regression somewhere. The current code here seems to assume that when nocap and inline image should be created.

knitr/R/hooks-md.R

Lines 58 to 62 in 46a48b1

if (is.null(w) && is.null(h) && is.null(s) && is.null(options$fig.alt) &&
a == 'default' && !(pandoc_html && in_bookdown)) {
# append <!-- --> to ![]() to prevent the figure environment in these cases
nocap = cap == '' && !is.null(to) && !grepl('^markdown', to) &&
(options$fig.num == 1 || ai) && !grepl('-implicit_figures', from)

I did look deeply through it yet but this gets me thinking 😅

Thanks for looking into it @atusy !

@atusy
Copy link
Collaborator Author

atusy commented Aug 16, 2021

@cderv
Thanks for your comments.
I understand your concern, and that is why I asked the question to yihui.
I also tried to figure out if the issue is some regression because the issue author started his comment with

I'm not sure when this started,

However, the behavior has been consistent for recent versions (I forgot which versions I tested...).
Well, I should re-examine the previous versions before making additional fixes.

The current patch fails to render captions with the following content, which misses blank lines after chunk.

```{r, fig.cap=c('foo', 'bar'), echo=FALSE}
plot(1)
plot(2)
```
end

@atusy
Copy link
Collaborator Author

atusy commented Aug 18, 2021

Closing as this PR is the duplicate of #1760

@atusy atusy closed this Aug 18, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2022
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.

3 participants