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

ajs journal included #437

Merged
merged 15 commits into from
Sep 9, 2021
Merged

ajs journal included #437

merged 15 commits into from
Sep 9, 2021

Conversation

matthias-da
Copy link
Contributor

@matthias-da matthias-da commented Sep 7, 2021

To contribute a new article template to this package, please make sure you have done the following things (note that journalname_article below is only an example name):

  • This project uses a Contributor Licence Agreement (CLA) that you'll be asked to sign when opening a PR. This is required for a significant pull request (it is fine not to sign it if a PR is only intended to fix a few typos). We use a tool called CLA assistant for that.
    You could also, unless you have done it in any other RStudio's projects before, sign the individual or corporate contributor agreement. 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/

Thank you!

@CLAassistant
Copy link

CLAassistant commented Sep 7, 2021

CLA assistant check
All committers have signed the CLA.

@cderv
Copy link
Collaborator

cderv commented Sep 7, 2021

Hi @matthias-da

Thanks for the contribution. I am preparing a release for this week, so timing is right. I will review this and hopefully we can end the work quicly.

To ease the review, I'll merge master into this branch to get the correct diff. Some changes are not right currently.

Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

Before going further into the review, I need to clarify something.

I see this is a template similar to JSS article: jss_article().

What is exactly the difference between both format ?

It seems we currently have duplication between both and only the documentclass is changing. Am I right ? Is there more ?

Thanks.

DESCRIPTION Outdated Show resolved Hide resolved
inst/rmarkdown/templates/ajs/template.yaml Outdated Show resolved Hide resolved
@matthias-da
Copy link
Contributor Author

Before going further into the review, I need to clarify something.

I see this is a template similar to JSS article: jss_article().

What is exactly the difference between both format ?

It seems we currently have duplication between both and only the documentclass is changing. Am I right ? Is there more ?

Thanks.

Indeed I with Achim Zeileis at the same University for some time and with his full commitment the Austrian Journal of Statistics uses only a slightly modified version of the Journal of Statistical Software (than Achim made) since 2013.
It's only about 20 lines that differs in the 500 lines .cls file. It's great to have this for our traditional Journal (since the 60ties) from the Austrian Statistical Society. With such help we are able to run it open-access without fees.

@cderv
Copy link
Collaborator

cderv commented Sep 8, 2021

Thanks for the precision !

If this is only the cls file that change, we may think of something else for rticles than duplicating the entire function.

From what I understand and see from this PR:

  • ajs_article() is exactly the same as jss_article() : there is no difference in the R function, except the folder name for the template, right ?
  • It seems there is no difference in the Pandoc templates, between ajs/resources/template.tex and jss/resources/template.tex. Is that right ?
  • Resources are different :
    • ajs.bst
    • ajs.cls
    • Some png and pdf file used in the skeleton
  • There is a specific skeleton with some specific guideline. Right ?

This means that currently, you are able to use jss_article() with a ajs.cls using this YAML header

---
documentclass: ajs
output: rticles::jss_article
---

Do we agree on this ?

Before looking at solution, we must agree on the "problem" to solve here:

  • jss_article() works for AJS if you provide the right .cls and .bst file
  • Those resources are not included in the package already.
  • It would be great to be able to write for AJS without having to download the CLS file.

Am I missing something ?

From there, I see several solutions:

  • is this an issue to use the jss_article() but change the documentclass ? The purpose of this PR is to included the correct skeleton and resources inside the package, is that so ? We could try to provide a template folder for ajs but that would still use the jss_article() function as-is

  • Building on the above we could try provide a variant of jss_article() that would use the correct cls. Something like

    output: 
      jss_article:
        variant: ajs

    This would use the correct document class in the template.tex. We would need to make the resources available when creating the template.

  • ajs_article() is better to be explicit and similar to other journal. In that case, I believe we can just do

    ajs_article <- jss_article

    Because there is no differences between the two functions. It is just an alias. ajs_article() would be working the same and only the documentclass would have to be change in the Rmd.

Anyway, we should find a solution that suits your initial aim but we won't duplicate code the way it is currently done in the PR.

I need you to clarify the aim and the user experience. What is the desired behavior ?
After that, I'll do the correct modification in the code base.

Thanks for your help

@matthias-da
Copy link
Contributor Author

Dear Christophe,

thanks a lot for all this work. Currently, ajs has different .cls, skeleton (guidelines) and images. To your questions.

Before looking at solution, we must agree on the "problem" to solve here:

jss_article() works for AJS if you provide the right .cls and .bst file

correct

Those resources are not included in the package already.

I'm sure that I put them within the pull request under inst/rmarkdown/templates/ajs/skeleton/

It would be great to be able to write for AJS without having to download the CLS file.

Absolutely agree on this. Thus I would like to ask you to put them in inst/rmarkdown/templates/ajs/skeleton/. If I missed something on this, I'm happy to follow up on this.

Proposed solutions:

  1. To have a few lines of duplicated code. Everything will surely work and probably in future we differ to JSS also in template and jss_article(). This would be far the easiest solution (I would keep it rather simple) and we will be more flexible in future.

  2. In case you want to strictly avoid this, I would go for a solution for that users can use ajs_article() without knowing jss (we thanked in our guidelines to JSS and Achim)

ajs_article <- jss_article

I think we should strictly avoid to name jss in any call from users, i.e. many of our authors might be confused, and thus a solution like this in the following is not a the best one in my point of view:

output:
jss_article:
variant: ajs

It would be great to have ajs_article() independent from jss_article from the user perspective. What do you think? Thanks a lot.

@cderv
Copy link
Collaborator

cderv commented Sep 9, 2021

It would be great to have ajs_article() independent from jss_article from the user perspective. What do you think?

I understand the reason you are giving. We'll offer a ajs_article() format then.

In case you want to strictly avoid this,

It is really not good practice to exactly duplicate the code of JSS. If the same behavior is required, then we'll use the function under-the-hood. The user does not need to know. If we need a different behavior, then we need to refactor into shared functions. But we don't want to have copy pasted part (which here is a whole file) in our code base.

Do you want to try to adapt the PR this way, avoiding the duplication ? I believe this is possible unless I missed something. This could require an adaptation of the current function though so that jss_article() can take an argument to choose the template, or we need to refactor the internals to use it in both jss_article() and ajs_article() - the later is probably better practice.

I can take it from here if you prefer.

@matthias-da
Copy link
Contributor Author

I can take it from here if you prefer.

You are much more into the code and package of rticles, so this would be great, and probably less work compared to answer my potential difficulties and follow-up questions. Thanks a lot.

@cderv cderv self-assigned this Sep 9, 2021
Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

@matthias-da

I have rework this branch. Can you test is and tell me if that works ok for you ?

remotes::install_github("rstudio/rticles#437")
# or
pak::pak("rstudio/rticles#437")

Also I left a few comments below for you to answer to know if we should modify or not before merging.

inst/rmarkdown/templates/ajs/skeleton/skeleton.Rmd Outdated Show resolved Hide resolved
inst/rmarkdown/templates/ajs/skeleton/skeleton.Rmd Outdated Show resolved Hide resolved
inst/rmarkdown/templates/ajs/skeleton/skeleton.Rmd Outdated Show resolved Hide resolved
@matthias-da
Copy link
Contributor Author

I just tested it. Looks great and does the job. Thanks a lot Christophe @cderv !

For the small issues regarding \section{}, href and preamble: of course, all ok (and better).
I guess it's easier for you to change these three minimal issues as to handle a new pull request?

@cderv
Copy link
Collaborator

cderv commented Sep 9, 2021

guess it's easier for you to change these three minimal issues as to handle a new pull request?

Those suggestion are made in the review so that you can easily accept them normally.
http://haacked.com/archive/2019/06/03/suggested-changes/

I'll add them.

@cderv cderv merged commit 2be3db1 into rstudio:master Sep 9, 2021
@cderv
Copy link
Collaborator

cderv commented Sep 9, 2021

Thanks @matthias-da !

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 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