Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

do not wrap HTML as it was done by default before Pandoc 2.17 #1305

Merged
merged 5 commits into from
Jan 24, 2022

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Jan 24, 2022

as this breaks our parsing process for section references.

This fixes #1301

THis PR is for now limited to the related issue but there are maybe other processing affected.

as this breaks our parsing process for section references.
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.

Thanks!

@cderv
Copy link
Collaborator Author

cderv commented Jan 24, 2022

Thanks for the quick review. I have just added a test so that we can detect some issue in the future. More tests like this would be required IMO. It would be great to detect Pandoc's breakage earlier. 😅

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.

Yes, we definitely need more tests.

R/ebook.R Outdated
Comment on lines 89 to 90
c(pandoc_args, '--section-divs', '--mathjax', '--number-sections',
if (rmarkdown::pandoc_available("2.17")) c('--wrap', 'none'))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
c(pandoc_args, '--section-divs', '--mathjax', '--number-sections',
if (rmarkdown::pandoc_available("2.17")) c('--wrap', 'none'))
c(pandoc_args2(pandoc_args), '--section-divs', '--mathjax', '--number-sections')
)

This also work it seems. Is it better as we already use --wrap elsewhere ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have made the change

cderv added 2 commits January 24, 2022 15:58
and fix issue in helper
instead of specific case for Pandoc 2.17
@cderv
Copy link
Collaborator Author

cderv commented Jan 24, 2022

And tests were failing 😞 ...

Snapshot test are really useful with markdown output as we can see clearly how it looks. However, maintaining several pandoc version is painful as the output will heavily change depending on the default behavior. So I went back to regex matching.
maybe using xml2 parsing would be the safest to target specific item and avoid regex.

I also use pandoc_args2() as it seems more relevant here as we are using it for other conversion in bookdown.

Does it seems better now ?

[skip ci]
@cderv
Copy link
Collaborator Author

cderv commented Jan 24, 2022

Does it seems better now ?

I think it is. I am merging.

@cderv cderv merged commit 6931d14 into main Jan 24, 2022
@cderv cderv deleted the pandoc2.17-html-wrapping branch January 24, 2022 15:24
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 27, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pandoc 2.17.0.1 can't find labels for labeled heading, so break their references
2 participants