Skip to content

Address review #60

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Address review #60

wants to merge 4 commits into from

Conversation

wright13
Copy link
Collaborator

Created vignette from paper excerpt and added unit tests. Unit test coverage is ~47%, but I didn't want to write a bunch of tests for the functions in updateR.R since I am actively reworking that code. Tidyverse test coverage is 53%, so I think we're in okay shape for now.

@wright13 wright13 requested a review from RobLBaker April 22, 2025 21:48
@RobLBaker
Copy link
Member

While I generally like the idea of including the paper in the package, I haven't been able to get the .rmd to properly knitt previously, especially getting multiple author and institution attributions (at least to .pdf). I wonder if it might be better to wait until the paper is published and then add the .pdf to the package?

@wright13
Copy link
Collaborator Author

@RobLBaker so this is how tidyverse is doing it: https://github.com/tidyverse/tidyverse/tree/main/vignettes. I think we'll have an easier time knitting to html than to pdf, if we do want to include the paper in the pkgdown site and/or as a vignette.
I do agree with the reviewer comment (openjournals/joss-reviews#8066 (comment)) that if folks are trying to learn about the package through the vignette instead of the readme/github pages, they currently aren't getting the information they need.
My thinking was that adding some info from the paper to the vignette is an easy way to make it more usable in the short term, and then once the paper is published we may want to talk more in depth about what belongs in the readme vs vignettes vs both, and also consider offering the full paper as a vignette. That said, I'm not tied to that strategy and we can definitely leave the vignette out for now if you'd rather.

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