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

Allowing references in R Markdown to PDF vignettes #95

Merged
merged 62 commits into from
Apr 13, 2023

Conversation

grimbough
Copy link
Collaborator

@grimbough grimbough commented Apr 6, 2022

This primarily addresses #94 where citations can't be used inside an R Markdown vignette using BiocStyle::pdf_document()

I've added a new pandoc template that includes the missing latex environments CSLReferences and cslreferences required by citeproc. I've passed this on similar features found in the tufte package. Interestingly rmarkdown now seems to use the templates shipped with pandoc, and maybe that would also be a way to deal with this, but the current approach seems to work.

I've also added a GitHub workflow to build the R Markdown vignette with various versions of Pandoc and TinyTex. I've also added a citation to the Rmd vignette so that the referencing infrastructure can be tested. This workflow would fail before my patch here is applied.


I've also made three small changes to Bioconductor.sty to suppress some warnings that are always produced, but which I don't think are informative to an end user as they can't do anything to change them. These were:

Warning messages:
LaTeX Warning: You have requested package `/BiocStyle/resources/tex/Bioconductor',
               but the package provides `Bioconductor'.
Package Fancyhdr Warning: \fancyhead's `E' option without twoside option is use less on input line 173.
Package geometry Warning: Over-specification in `h'-direction. `width' (384.1122pt) is ignored.

I can restore the if they may be useful, but I've always been slightly annoyed to see them.
I don't see an easy way to fix the first error, so I just suppress it with the silence package. I've reverted this, it was breaking the Sweave vignette.
The second might influence the layout of a two-sided PDF, but I'm not sure it's actually possible to produce one of those with BiocStyle.
The third states it is ignored, so I guess it doesn't do anything and I've removed the line that leads to it.

@grimbough
Copy link
Collaborator Author

Thanks @aoles I'd happy recieve a review now.

The second set of commits address an issue I've noticed recently where newer versions of pandoc add some additional classes to the footnote div and the existing code doesn't find them and move them to the margins. I've added a unit tests and a github workflow that uses a variety of Pandoc versions to try and tests for this. You can see and R CMD check that fails with Pandoc 2.17 at https://github.com/grimbough/BiocStyle/actions/runs/2436503421 and then after my patch at https://github.com/grimbough/BiocStyle/actions/runs/2436503421

Copy link
Collaborator

@aoles aoles left a comment

Choose a reason for hiding this comment

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

Hi Mike!

Thanks again for the improvements to the handling of citations in BiocStyle. After a quick look I have the following initial ideas and suggestions:

  1. I was a bit surprised to see that by default pandoc-generated citations are not hyperlinked. As all of the other references such as to figures, tables and equations, as well as footnotes are linked, I think a reasonable thing would be to enable citations to render as links as well. For the html output it might be enough to have the YAML parameter link-citations: yes set by default. As for the pdf output, I'm not sure what the best approach would be, but I believe this should be feasible too. Is this something you would be willing to look into?
  2. Also, the way how the bibliography in the "Authoring R Markdown vignettes" vignette shows up appended directly after the output of sessionInfo() looks somehow weird to me. Do you think you could improve on this? What I could imagine is a similar structure as in the "Bioconductor LATEX Style 2.0" vignette: have "References" as an unnumbered section, and move "Session info" to an Appendix, cf. here.

Cheers,
Andrzej

@grimbough grimbough self-assigned this Jul 21, 2022
@grimbough
Copy link
Collaborator Author

Thanks for the review @aoles, sorry I haven't replied until now. I'm currently on parerntal leave for a few months and had too many things to tidy up before I stopped work. I'll happily take a look at making the suggested changes when I get some time, but it might be a while.

Merge branch 'master' of git.bioconductor.org:packages/BiocStyle

# Conflicts:
#	DESCRIPTION
@grimbough
Copy link
Collaborator Author

2. Also, the way how the bibliography in the "Authoring R Markdown vignettes" vignette shows up appended directly after the output of sessionInfo() looks somehow weird to me. Do you think you could improve on this? What I could imagine is a similar structure as in the "Bioconductor LATEX Style 2.0" vignette: have "References" as an unnumbered section, and move "Session info" to an Appendix, cf. here.

Hi @aoles For the point above, do we achieve the formatting programmatically, or is just that the default template for the BiocStyle sweave format has a better example layout and so achieve a nicer end product, but someone could still make an ugly document if they tried?

@grimbough
Copy link
Collaborator Author

Hi @aoles ,

I finally got round to addressing your review.

  1. Turns out setting link-citations: yes is sufficient for links to work in PDF output too, so that was an easy fix. I've also put that in the header of the Rmd vignette and the two document skeletons, so it will be the default going forward.

  2. I've restructured the vignette and document skeletons to include a # References section before the sessionInfo(). I also modified the pre_processor() function to look for that exact section name, and automatically insert the weird <div class='refs> and the # (APPENDIX) Appendix {-} so that people don't have to go hunting for that syntax. It's not exactly easy to remember. The PDF and HTML versions of the vignette now render like this:

Selection_003
Selection_002

Not sure why there isn't a heading for the Appendix in the PDF version, but generally I think it looks much more structured than before.

Thanks for taking the time to take a look. It's probably be good to get these merged into the main if you like them. My fork is diverging quite far now!

Copy link
Collaborator

@aoles aoles left a comment

Choose a reason for hiding this comment

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

Hi Mike,

Long time no see! I hope you enjoyed your parental leave. Let me apologize for not getting back to you earlier, the end of last year was quite a busy time for me.

Thanks a lot for working on this and for addressing my comments. This looks really great now, keep up the good work! I still have some minor improvement suggestions, please find them in the inline comments.

Cheers!
Andrzej

@aoles
Copy link
Collaborator

aoles commented Apr 4, 2023

Hi Mike @grimbough,

I hope you are doing fine. I was wondering whether it would be feasible to wrap-up the review soonish in order to merge before the upcoming BioC release? Please let me know if there is anything I can help you with.

Looking forward to hearing from you.

Cheers!
Andrzej

@aoles aoles merged commit 6c7b120 into Bioconductor:devel Apr 13, 2023
@aoles
Copy link
Collaborator

aoles commented Apr 13, 2023

Thanks @grimbough for all your nice work! 👍 I appreciate you efforts in maintaining BiocStyle ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants