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

Update manuscript to reference correct object names and paper_id #11

Merged
merged 9 commits into from
Sep 6, 2022

Conversation

egouldo
Copy link
Member

@egouldo egouldo commented Sep 5, 2022

  • Manuscript has been updated to reference the anonymised paper ID's and names to old supp. data objects have been changed to reflect current names.
  • Manuscript files moved to ./vignettes/, BUT, I can't get the vignette to register when calling browseVignettes(package = "aggreCAT"). @psychtek can you please make any necessary changes to the draft PR so the vignette is included in the package build?

@egouldo egouldo requested a review from psychtek September 5, 2022 07:02
@psychtek psychtek self-assigned this Sep 5, 2022
@psychtek
Copy link
Collaborator

psychtek commented Sep 5, 2022

@egouldo No worries ill jump in and get this branch rolling 🥼

@psychtek
Copy link
Collaborator

psychtek commented Sep 6, 2022

Ok so I am getting conflicting information when I start digging around as to where to put manuscript docs. Seems that the vignettes is for package reference vignettes? Manuscripts go into sub folder in inst/docs. Confused 😅
https://stackoverflow.com/questions/59645828/how-to-properly-organise-vignettes-and-inst-folders-when-creating-new-r-package

We might need to create a reference pdf of the package anyway?

My understanding is that we create the rmd in the vignettes folder.
then devtools::build_vignettes() which does a bunch of fancy stuff:

Copying vignettesMoving manuscript.html and manuscript.R to doc/Copying manuscript.Rmd to doc/Building vignette index

But this is for html.

@egouldo But the manuscript is printing to pdf and html correct?

If all we need is the pdf then theres info here about just placing the pdf and asis inside the vignettes.

Also, unless theres a more up to date link Quarto isnt currently supported for vignettes (at least for CRAN approval): r-lib/pkgdown#2154
EMODnet/EMODnetWCS#11
So we might need to change it back to Rmd format.

Good summary about vignettes and pdf articles here

@egouldo
Copy link
Member Author

egouldo commented Sep 6, 2022

Hmmm, sounds like there's also a difference between 'reference manual' and a 'vignette'. I think reference manual is cobbled together document of all the R man for the package, and vignette is a separate long-form document yet again.

@egouldo
Copy link
Member Author

egouldo commented Sep 6, 2022

For now, simplest way forward is to include the knitted quarto PDF of the manuscript rather than trying to build the .qmd document in the package building workflow.

See: https://cran.r-project.org/web/packages/R.rsp/vignettes/R_packages-Static_PDF_and_HTML_vignettes.pdf

  • Elliot to include PDF of manuscript as PDF

@egouldo
Copy link
Member Author

egouldo commented Sep 6, 2022

Ok @psychtek , so package builds locally, but fails check because r.sp is not specified as the package builder (had to change it from knitr to r.sp (See 9a79e4f, file: DESCRIPTION). That's the vignette builder we need to include PDF as pkg vignette.

Log L#30: https://github.com/metamelb-repliCATS/aggreCAT/runs/8199741340?check_suite_focus=true#step:5:31

@psychtek
Copy link
Collaborator

psychtek commented Sep 6, 2022

Creeping every so closely 😄

@egouldo
Copy link
Member Author

egouldo commented Sep 6, 2022

Creeping every so closely 😄

😅 Do we need to update GH actions file or docker to make the checks work?

@psychtek
Copy link
Collaborator

psychtek commented Sep 6, 2022

Nah its neeeearly there! I have set it up to flag when warnings are present

checking for portable file names ... WARNING
  Found the following files with non-portable file names:
    inst/ms/_extensions/jss/_extensions/quarto-ext/fancy-text/_extension 2.yml
    inst/ms/_extensions/jss/_extensions/quarto-ext/fancy-text/fancy-text 2.lua

if these arent needed for ms build then add to .buildignore ?

@egouldo egouldo marked this pull request as ready for review September 6, 2022 05:54
@egouldo
Copy link
Member Author

egouldo commented Sep 6, 2022

ping @psychtek ready for review, we all ✅ on Rcmdcheck

Copy link
Collaborator

@psychtek psychtek left a comment

Choose a reason for hiding this comment

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

      - name: vignette-builder
        run:  install.packages("R.rsp"); remotes::install_deps(dependencies = TRUE)
        shell: Rscript {0}

Ill bake this into the image at a later date.

@psychtek psychtek merged commit 906750b into master Sep 6, 2022
@psychtek psychtek deleted the ms-anon-data branch September 6, 2022 06:03
@egouldo
Copy link
Member Author

egouldo commented Oct 11, 2022 via email

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