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.

Sign up for GitHub

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

Jump to bottom

Mdpi update #515

Merged
merged 44 commits into from
Apr 25, 2023
Merged

Mdpi update #515

merged 44 commits into from
Apr 25, 2023

Conversation

mps9506
Copy link
Contributor

@mps9506 mps9506 commented Feb 4, 2023 •
edited
Loading

This is a fresh pull request with the newest version of the mdpi template circa 2022-12 (PR #381 seemed to stall out and was already outdated). There are numerous updates to the yaml header to make better use of the mdpi.cls and template file.
Also fixes #502

How to contribute a new output format ?

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). Unless you have done it in any other RStudio / Posit projects before, sign the individual or corporate contributor agreement as appropriate. You can send the signed copy to [email protected].

  • Add the journalname_article() function to R/article.R if the output format is simple enough, otherwise create a separate R/journalname_article.R.

  • Document your function using roxygen2. Markdown syntax is supported. Refer to https://roxygen2.r-lib.org/articles/rd-formatting.html for formatting references.

  • 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!

@rempsyc
Copy link

rempsyc commented Feb 18, 2023

There is a MDPI special issue deadline next Thursday. It would be awesome if this PR could be merged before then :-)

@rempsyc rempsyc mentioned this pull request Feb 25, 2023
Merged
cderv
cderv requested changes Apr 17, 2023
View reviewed changes
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.

Thanks a lot for this PR. Sorry for the delay. I am going back on rticles now.

I have a few comments and questions.

If you don't have time anymore on this, I can understand. I'll do the change myself if so. Please do tell me.

Thank you

inst/rmarkdown/templates/mdpi/skeleton/skeleton.Rmd Outdated Show resolved Hide resolved
inst/rmarkdown/templates/mdpi/resources/template.tex Outdated
Comment on lines 5 to 10
% pandoc conditionals added to preserve backwards compatibility with previous versions of rticles
$if(classoption)$
\documentclass[$for(classoption)$$classoption$$sep$,$endfor$]{$cls$}
$else$
\documentclass[$journal$,$type$,$status$,moreauthor,pdftex]{$cls$}
$endif$
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand the change, though I wonder if we shouldn't mix both.

It seemed useful to not rely of the order of classoption but use named field in YAML for the expected option by mdpi.cls

We can do both either from root yaml or ven something more targeted to format function

output:
  mdpi_article:
    journal: ...

Just curious if you think that letting classoption be free is really best for users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having the open classoption felt more consistent with what I saw in other templates so I adopted it. I think I agree that using named fields makes sense here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can do both

  • Named field to make it easier for authors, and insure correct order
  • classoption for further customization

We could do

journal: sports
type: article
status: submit
classoption:
  - moreauthors # use when multiple author
  - pdftex # use when using latex_engine pdflatex
output: mdpi_article

Or set everything under mdpi_article directly, not sure which is better.

We could probably also handle moreauthors and pdftex insertion from R format function, by (1) checking if multiple authors in yaml header and (2) checking latex_engine value

Does it sounds helpful ?

I am also fine with leaving classoption free. Easier to maintain, but more responsability for authors who need to know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moreauthors and pdftex are now set by the format function so are not needed in the yaml header. journal, type, and status are named fields. classoption is no longer needed in this case.

inst/rmarkdown/templates/mdpi/resources/template.tex Outdated Show resolved Hide resolved
inst/rmarkdown/templates/mdpi/resources/template.tex Outdated Show resolved Hide resolved
inst/rmarkdown/templates/mdpi/resources/template.tex Show resolved Hide resolved
inst/rmarkdown/templates/mdpi/skeleton/skeleton.Rmd Outdated Show resolved Hide resolved
inst/rmarkdown/templates/mdpi/skeleton/skeleton.Rmd Outdated Show resolved Hide resolved
inst/rmarkdown/templates/mdpi/skeleton/skeleton.Rmd Outdated Show resolved Hide resolved
inst/rmarkdown/templates/mdpi/skeleton/skeleton.Rmd Show resolved Hide resolved
inst/rmarkdown/templates/mdpi/skeleton/skeleton.Rmd Outdated Show resolved Hide resolved
@cderv cderv mentioned this pull request Apr 17, 2023
Closed
1 task
@cderv cderv linked an issue Apr 17, 2023 that may be closed by this pull request
Template in mdpi_template is outdated #369
Closed
3 tasks
@mps9506
Copy link
Contributor Author

mps9506 commented Apr 17, 2023

@cderv I'll work on the changes.

@cderv
Copy link
Collaborator

cderv commented Apr 17, 2023

Awesome. Do not hesitate to tell me if you need me to help or if I should do it instead.

@cderv cderv mentioned this pull request Apr 18, 2023
Open
@mps9506 mps9506 marked this pull request as draft April 18, 2023 20:48
mps9506
mps9506 commented Apr 19, 2023
View reviewed changes
R/article.R Show resolved Hide resolved
@mps9506 mps9506 marked this pull request as ready for review April 20, 2023 12:08
@rstudio rstudio deleted a comment from CLAassistant Apr 20, 2023
@cderv
Copy link
Collaborator

cderv commented Apr 20, 2023

Thanks for the work @mps9506 ! I'll review it now.

As stated in the PR template, can you be sure to sign the CLA document and send it to the email ?

Thank you

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). Unless you have done it in any other RStudio / Posit projects before, sign the individual or corporate contributor agreement as appropriate. You can send the signed copy to [email protected].

cderv
cderv approved these changes Apr 20, 2023
View reviewed changes
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.

Thanks - this is really good PR !

I'll do a few tweaks myself, and then merge once you have confirmed me you sent the CLA document.

Thank you again !

@cderv
Copy link
Collaborator

cderv commented Apr 20, 2023

@mps9506 do you have aslo these warnings locally ?

Warning: Package fancyhdr Warning: \headheight is too small (12.0pt):
Warning: (fancyhdr) Make it at least 20.0pt, for example:
Warning: (fancyhdr) \setlength{\headheight}{20.0pt}.
Warning: (fancyhdr) You might also make \topmargin smaller to compensate:
Warning: (fancyhdr) \addtolength{\topmargin}{-8.0pt}.
Warning: Package fancyhdr Warning: \headheight is too small (12.0pt):
Warning: (fancyhdr) Make it at least 16.18796pt, for example:
Warning: (fancyhdr) \setlength{\headheight}{16.18796pt}.
Warning: (fancyhdr) You might also make \topmargin smaller to compensate:
Warning: (fancyhdr) \addtolength{\topmargin}{-4.18796pt}.

Is this by default with their template ?

@mps9506
Copy link
Contributor Author

mps9506 commented Apr 20, 2023

@cderv Those warnings are issues in mdpi cls or template file. I get them when compiling the template directly in TexLive. I'll send over the CLA shortly.

@cderv
Copy link
Collaborator

cderv commented Apr 20, 2023

Those warnings are issues in mdpi cls or template file.

ok good to know. Hopefully they'll fix them

@cderv
Copy link
Collaborator

cderv commented Apr 21, 2023

All good now. Just waiting for confirmation of the CLA and then I'll merge

@mps9506
Copy link
Contributor Author

mps9506 commented Apr 23, 2023

@cderv I sent over the CLA. Just a note that the repository template file for contributors has the [email protected] address while the actual CLA form indicates to send it to [email protected]. I copied both since I was unsure.

@cderv
Copy link
Collaborator

cderv commented Apr 25, 2023 •
edited
Loading

Oh thanks. I'll check that. Merging now then !

Thanks a lot for the work on this !

@cderv cderv merged commit e6241e3 into rstudio:main Apr 25, 2023
cderv added a commit that referenced this pull request May 11, 2023
@mps9506 @cderv
Mdpi update (#515)
e8c7d14 
Co-authored-by: Michael Schramm <[email protected]>
Co-authored-by: Christophe Dervieux <[email protected]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cderv cderv cderv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
R Markdown Team Projects
Archived in project
Milestone
No milestone
3 participants
@mps9506 @rempsyc @cderv