add LIPIcs article format, closes #232 by nuest · Pull Request #288 · rstudio/rticles (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation57 Commits8 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

@yihui Here's another template :-)

If you have any idea why my local system does not find the plainurl.bst, I'm happy to remove that file, but it needed to be there for me to work.

Unless you have done it in any other RStudio's projects before, please sign the individual or corporate contributor agreement for a significant pull request (it is fine not to sign it if a PR is only intended to fix a few typos). You can send the signed copy to jj@rstudio.com.
Add the journalname_article() function to R/article.R if the output format is simple enough, otherwise create a separate R/journalname_article.R.
Add the Pandoc LaTeX template inst/rmarkdown/templates/journalname/resources/template.tex.
Add a skeleton article inst/rmarkdown/templates/journalname/skeleton/skeleton.Rmd.
Add a description of the template inst/rmarkdown/templates/journalname/template.yaml.
Please include the document class file (*.cls) if needed, but please do not include standard LaTeX packages (*.sty) that can be downloaded from CTAN. If you are using TinyTeX or TeX Live, you can verify if a package is available on CTAN via tinytex::parse_packages(files = "FILENAME"") (e.g., when FILENAME is plain.bst, it should return "bibtex", which means this file is from a standard CTAN package). Please keep the number of new files absolutely minimal (e.g., do not include PDF output files), and also make examples minimal (e.g., if you need a .bib example, try to only leave one or two bibliography entries in it, and don't include too many items in it without using all of them).
Update Rd and namespace (could be done by devtools::document()).
Update NEWS.
Update README with a link to the newly supported journal. Please add your Github username and the full name of the journal (follow other examples in the list).
Add a test to tests/testit/test-formats.R by adding a line test_format("journalname"). We try to keep them in alphabetical order.
Add your name to the list of authors Authors@R in DESCRIPTION. You don't need to bump the package version in DESCRIPTION.

Lastly, please try your best to do only one thing per pull request (e.g., if you want to add two output formats, do them in two separate pull requests), and refrain from making cosmetic changes in the code base: https://yihui.name/en/2018/02/bite-sized-pull-requests/

nuest marked this pull request as draft

July 7, 2020 09:22

I am now going through the actual submission and realize, that there are some things that must be configurable in addition.

nuest marked this pull request as ready for review

July 7, 2020 09:51

Choose a reason for hiding this comment

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

Thank you very much! Could you go through the checklist before we actually review the PR? https://github.com/rstudio/rticles/blob/master/.github/PULL_REQUEST_TEMPLATE.md (e.g. if CHANGELOG.md is not required for the template to work, you may leave it out)

BTW, if you are using TinyTeX, it should be able to find the correct package to install:

> tinytex::parse_packages(files = 'plainurl.bst')
tlmgr search --file --global '/plainurl.bst'
[1] "urlbst"

Collaborator

@cderv cderv left a comment • Loading

Choose a reason for hiding this comment

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

Hi @nuest !

Thanks a lot for your contribution. I posted a few comments some of which need to be dealt with, the other are for potential improvements if you think it is correct.

General comment :

There will be a conflict with the README because we change it recently. I will deal later when ready to merge - don't worry.
Are CHANGELOG.md and LICENCE.md necessary to be included in rticles ?
If I delete the plainurl.bst file from the created directory, it renders fine. I don't think this is useful. I am using TinyTex though. What is your issue with this file ?

I also let you go through the PR checlklist by yourself (you can edit you first post, copy the content and check the boxes when ready). Thank you.

Thanks for the review @cderv I'll get to this hopefully by the end of the week.

I've copied the new PR checklist over and think I covered all the requested changes. Remaining tasks

use new filter for corollary and proposition
fix plainurl.bst

However, for me it does not work when I remove plainurl.bst. I'm also using TinyTex (AFAICT?):

/usr/bin/pandoc +RTS -K512m -RTS MyArticle.utf8.md --to latex --from markdown+tex_math_single_backslash-autolink_bare_uris-auto_identifiers --output MyArticle.tex --self-contained --template /home/daniel/R/x86_64-pc-linux-gnu-library/4.0/rticles/rmarkdown/templates/lipics/resources/template.tex --highlight-style tango --pdf-engine xelatex --natbib --lua-filter /home/daniel/R/x86_64-pc-linux-gnu-library/4.0/rmarkdown/rmd/lua/pagebreak.lua --lua-filter /home/daniel/R/x86_64-pc-linux-gnu-library/4.0/rmarkdown/rmd/lua/latex-div.lua 
This is BibTeX, Version 0.99d (TeX Live 2020)
The top-level auxiliary file: MyArticle.aux
I couldn't open style file plainurl.bst
---line 20 of file MyArticle.aux
 : \bibstyle{plainurl
 :                   }
I'm skipping whatever remains of this command
I found no style file---while reading file MyArticle.aux
(There were 2 error messages)
Error: Failed to build the bibliography via bibtex

I have no idea what's going wrong here, thankful for any ideas.

plainurl.bst is part of urlbst package

tinytex::parse_packages(files = "plainurl.bst") tlmgr search --file --global "/plainurl.bst" [1] "urlbst"

But when this package is not installed and the file is not present, it does not throw an error inside TeX log, so the package is not install automatically by TinyTeX.

If we install the package, the Rmd compile correctly even if plainurl.bst is not in the folder.

installed by

tinytex::tlmgr_install("urlbst")

then re render

either tinytex package is missing a way to handle those missing package containing bst file.
either we need to say to your article format that this package is required for tinytex to install it if missing. But it does not seem as a usual TeX package you can pass in extra_dependencies (https://ctan.org/pkg/urlbst).
either we shipped one version of the bst file with the skeleton.

@yihui is tinytex only parsing missing dependency from the log file ? I don't see any mention in this case about the missing plainurl.bst file that would trigger the installation from after tinytex::parse_package ?
You more expert than me on TeX Live - how should this work for bst file included in CTAN package ?
Thank you.

Also @nuest , don't forget at the end to remove the MyArticle folder that I guess you are using to test ? It should not be commited. 😄

@cderv Thanks for the investigation. PR updated without the test artefact. Let's see what @yihui has to say about the missing file.

All committers have signed the CLA.

I can confirm that the file plainurl.bst is not needed when urlbst is installed.

A hacky workaround for the urlbst package issue would be to add a snippet to the skeleton.Rmd that checks if urlbst is installed and if not it installs it:

if( !"urlbst" %in% tinytex::tl_pkgs()) tinytex::tlmgr_install("urlbst")

I would add a note that authors may remove the chunk after the first rendering.

I briefly checked extra_dependencies (didn't know that option), but that only leads to an error for me (the same for you @cderv ?): "! LaTeX Error: File urlbst.sty' not found." - I think that mechanism does not work for .bst` files?

I've taken a look at the tinytex source code, and it was straightforward to let detect_files(..) detect the log "I couldn't open style file plainurl.bst" BUT the problem is that this log is not in the .log file somehow. Could the problem be that the error message is earlier in the process?

Here is a screenshot of the relevant log output:

I have switched a bunch of custom commans over to fenced blocks, and it works nicely except for the ones where the name ends in *:

  ::: {.remark* data-latex=""}
  Fusce eu leo nisi. Cras eget orci neque, eleifend dapibus felis. Duis et leo dui. Nam vulputate,   velit et laoreet porttitor, quam arcu facilisis dui, sed malesuada risus massa sit amet neque.
  :::
  
  ::: {.claim data-latex=""}
  \label{testenv-claim}
  
  Fusce eu leo nisi. Cras eget orci neque, eleifend dapibus felis. Duis et leo dui. Nam vulputate,   velit et laoreet porttitor, quam arcu facilisis dui, sed malesuada risus massa sit amet neque.
  :::
  
  ::: {.claim* data-latex=""}
  \label{testenv-claim2}
  
  Fusce eu leo nisi. Cras eget orci neque, eleifend dapibus felis. Duis et leo dui. Nam vulputate,   velit et laoreet porttitor, quam arcu facilisis dui, sed malesuada risus massa sit amet neque.
  :::

Gives me

@cderv Would rstudio/bookdown#940 be an appropriate place to report this? Do you think lemma* could be supported as a block name?

@cderv Would rstudio/bookdown#940 be an appropriate place to report this? Do you think lemma* could be supported as a block name?

Yes this would be a good place to support this feature. Do you want unnumbered Lemma, is that it ?

Bookdown has a special support for theorem and proof env, and we are trying to make it work also with fenced div syntax for those specific environments.

Without bookdown, it is just the rmarkdown support for latex-divs. Can you report the bug there too about the star syntax not working ?

Also, seeing

  ::: {.claim data-latex=""}
  \label{testenv-claim}
  
  Fusce eu leo nisi. Cras eget orci neque, eleifend dapibus felis. Duis et leo dui. Nam vulputate,   velit et laoreet porttitor, quam arcu facilisis dui, sed malesuada risus massa sit amet neque.
  :::

I am thinking we could improve the lua filter by adding the \label line base on a provided id. That would make this works as you expect I think

::: {.claim #testenv-claim data-latex=""}
Fusce eu leo nisi. Cras eget orci neque, eleifend dapibus felis. Duis et leo dui. Nam vulputate,   velit et laoreet porttitor, quam arcu facilisis dui, sed malesuada risus massa sit amet neque.
:::

would be

\begin{claim} \label{testenv-claim} Fusce eu leo nisi. Cras eget orci neque, eleifend dapibus felis. Duis et leo dui. Nam vulputate, velit et laoreet porttitor, quam arcu facilisis dui, sed malesuada risus massa sit amet neque. \end{claim}

Do you think it is a good idea ?

@yihui Regarding the blg file, the error we got with this template comes in fact from build_bib() just before check_blg(). That is why it is not working because it errors, and the blg file is not parsed as the function exits.
https://github.com/yihui/tinytex/blob/9b48862b90a47d2f69b7e80b421ce1e3d65a2a91/R/latex.R#L235-L237
But from the log below, we see that the correct sentence is output in the log "I couldn't open style file plainurl.bst"

rmarkdown::render("Untitled/Untitled.Rmd", clean = FALSE) (...) This is BibTeX, Version 0.99d (TeX Live 2020/W32TeX) The top-level auxiliary file: Untitled.aux I couldn't open style file plainurl.bst ---line 20 of file Untitled.aux : \bibstyle{plainurl : } I'm skipping whatever remains of this command I found no style file---while reading file Untitled.aux (There were 2 error messages) Erreur : Failed to build the bibliography via bibtex traceback() 7: stop("Failed to build the bibliography via ", bib_engine, call. = FALSE) 6: system2_quiet(bib_engine, shQuote(aux), error = { stop("Failed to build the bibliography via ", bib_engine, call. = FALSE) }) 5: build_bib() 4: latexmk_emu(file, engine, bib_engine, engine_args, min_times, max_times, install_packages, clean) 3: tinytex::latexmk(file, engine, if (biblatex) "biber" else "bibtex") 2: latexmk(texfile, output_format$pandoc$latex_engine, "--biblatex" %in% output_format$pandoc$args) 1: rmarkdown::render("Untitled/Untitled.Rmd", clean = FALSE)

So with no error, it should work as expected. But it seems to error because it does not find the .bst file.

Should this error be quiet for build_bib() and check_blg be run on the .blg file with the error ?
I can confirm that check_blg() works on the produced blg file - it is just not read due to the error.

@cderv Thanks for the investigation! I see what's wrong now, and will fix it in tinytex soon. For this PR, please ignore this particular issue and don't include plainurl.bst.

yihui added a commit to rstudio/tinytex that referenced this pull request

Sep 28, 2020

…tex error occurs, should not stop immediately but try to parse and install missing packages first

The .bst issue should be fixed in tinytex now.

remotes::install_github('yihui/tinytex')

tinytex::tlmgr_remove('urlbst')

rmarkdown::render("Untitled/Untitled.Rmd")

It should automatically install urlbst.

@yihui I can confirm the problem disappeared with tinytex 0.26.3. Thanks @cderv for the hard work of pinpointing the problems.

@cderv :

I think making the label an explicit option would be a nice added feature, though I don't think is is strictly needed for this PR, right?
Yes, unnumbered blocks, i.e. ones with names ending in *, is what this template needs to not use any LaTeX at all for custom environments. Not sure either if this PR should wait for that. Opinion?

I think making the label an explicit option would be a nice added feature, though I don't think is is strictly needed for this PR, right?

No not needed

Yes, unnumbered blocks, i.e. ones with names ending in *, is what this template needs to not use any LaTeX at all for custom environments. Not sure either if this PR should wait for that. Opinion?

This would be something to support in latex-divs.lua in rmarkdown. One issue though as this is not recognized by Pandoc as a Fenced Div

::: {.claim* data-latex=""}
Something
:::

However, this would be recognized by pandoc, and we could detect the *

But, you can't pass data-latex="" so for now it is not processed by our Lua filter.

I don't think this PR should wait for that either. It works as is, you just need to use latex syntax for unnumbered environement.

What we could quickly do though is this syntax

::: {.claim .unnumbered data-latex=""}
Something
:::

If we detect the class unumbered within a div for latex, we would add the * to the environment name.

Is the star a common pattern for Latex environment ?

I'll review your last chance this week anyway - you could modify your template later. Is the PR ready ?

Yes, the PR is ready from my side.

Is the star a common pattern for Latex environment ?

It is used for \(sub, ..)section, \chapter, etc., so I'm inclined to say yes. I like your suggestion to use .unnumbered because it fits what you can do with chapters/sections.

Note the PR includes a small fix for the springer_article() function so R CMD check passes. I'm happy to mention that in the NEWS, if you see the need.

Collaborator

@cderv cderv left a comment • Loading

Choose a reason for hiding this comment

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

Hi @nuest,

Sorry for the delay.

Here are my last questions / comments of the changes I plan to push before merging.
We'll need also to depend on currently dev tinytex package but I'll add that before merging.

Are you ok with those changes ?

README.md Outdated Show resolved Hide resolved

- `\relatedversion` (if there is a related version, typically the ``full version''); please make sure to provide a persistent URL, e.\,g., at arXiv.
- `\begin{abstract}...\end{abstract}`.

\paragraph*{Please do not \ldots}

Choose a reason for hiding this comment

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

I believe you can write this also as markdown

Please do not \dots {.unumbered}

\paragraph*{Please do not \ldots}
#### Please do not \ldots {.unnumbered}

not what you want ? Just asking in case there is a reason

- Use pdflatex and an up-to-date \LaTeX{} system.
- Use further \LaTeX{} packages and custom made macros carefully and only if required.
- Use the provided sectioning macros: `\section` (`#` in Markdown), `\subsection` (`##` in Markdown), `\subsubsection` (`###` in Markdown),
`\paragraph`, `\paragraph`, and `\subparagraph`.

Choose a reason for hiding this comment

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

This can also be written in markdown

\paragraph => ####
\paragraph* => #### title {.unnumbered}
\subparagraph => #####
\subparagraph* => ##### title {.unnumbered}

out of curiosity is there a reason not to use those ?

fixes test error for springer_article()
update NEWS file with proper thanks

@cderv I've added your changes manually so I can update the single commit from my end. Thank you for your perseverance with this PR! I did a rebase so the merge should work well.

So it seems you've just forced push and we have now one commit. Some of the changes I suggested where not included. I'll handle that as you seem ok, and check our previous review comment.

If you just merge master in your branch, you can avoid to force-push and we can keep the previous commit in the PR history. When we'll merge in master, we will squash the change so that there will be one commit only in the master history. But I think it is great to keep PR will all the commits and iteration. No worries, just sharing my thoughts.

cderv mentioned this pull request

Oct 29, 2020

@nuest I reverted some changes regarding the environment syntax. As you see there is an issue if you want to use bookdown::pdf_book (rstudio/bookdown#1001) For now it will not work.

I reverted it so that we merge this, and if you desire we work on an update when everything is working.

Test locally and it is ok. (We have issue with our CI currently)

Thanks @cderv !

This really was a more complicated beast than I hoped for. I've now written two articles based on the template though, so the foundation seems to be alright. Please feel free to ping me when the bookdown problem is solved - I'm not sure I'll be able to follow the development over there.

This really was a more complicated beast than I hoped for.

That was worth it ! thank you for making this PR!

Please feel free to ping me when the bookdown problem is solved

Note that you can subscribe to an issue to reveive updates as Github notification. It is on the right column in the issue rstudio/bookdown#1001
Anyways, I'll try to remember and ping you.

clrpackages pushed a commit to clearlinux-pkgs/R-tinytex that referenced this pull request

Nov 5, 2020

…0.27

Alfonso Ruzafa (1): fix install-bin-unix script (#243)

Yihui Xie (28): make sure that I'll remember to remove the temporary patch for tlmgr.pl in the future (uname)hasbeenstoredin(uname) has been stored in (uname)hasbeenstoredinOSNAME if tinytex_root() returns '', there is no need to check further if this root directory is TinyTeX (it definitely isn't) need to group the latter two expressions fix a problem discovered at rstudio/rticles#288 (comment): when a bibtex error occurs, should not stop immediately but try to parse and install missing packages first use xfun::grep_sub() in a few other places xfun 0.18 is on CRAN now keep (cached) R packages up-to-date only update packages that can be possibly updated (e.g. on Linux, don't update /usr/lib/R/library) suppress the false alarm on Windows when running tlmgr path add: resolve action earlier don't set R_LIBS_USER if it has been set always set R_LIBS_USER on *nix close rstudio/tinytex-releases#9: remove %APPDATA\TinyTeX% twice like https://github.com/yihui/tinytex/blob/ce0e9d311830673dbfd2aa945145c54c45020b7f/tools/install-windows.bat#L34 because rd once can't really remove this dir sometimes forgot to add path delete install-tl and install-tl-windows.bat because users don't need them, and they can confuse users: rstudio/tinytex-releases#9 patch tlmgr.pl on Windows to avoid the false alarm: http://www.tug.org/svn/texlive/trunk/Master/texmf-dist/scripts/texlive/tlmgr.pl?r1=56562&r2=56561&pathrev=56562 add ... arguments to r_texmf() and tlmgr_conf() add/remove R's texmf tree quietly add the firstaid package as discovered in https://travis-ci.com/github/yihui/tinytex/jobs/396057138 one more package: https://travis-ci.com/github/yihui/tinytex/jobs/396082629 remove the patch introduced in d9ed3c452bbcb526ad682b18a99207d084ce1dbd; the problem seesm to have been fixed upstream the bug in tlmgr info --list --data relocatable has been fixed, so remove this patch, and add a test so that we will be able to discover this problem if it occurs again add a tlmgr_repo function to query or set CTAN repository import two more functions from xfun wrong logic here: the commit ffce7ab7152cb241d26a299208c8642fda0d204e should be correct; not sure how I screwed it up later fix #252: for Linux machines, the arch must be x86_64 for the prebuilt binary version of TinyTeX to be installed; if it is not x86_64, install from source CRAN release v0.27

cderv mentioned this pull request

Apr 23, 2021

github-actions bot locked as resolved and limited conversation to collaborators

May 3, 2021