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

Add cff #1

Merged
merged 5 commits into from
May 1, 2024
Merged

Add cff #1

merged 5 commits into from
May 1, 2024

Conversation

c-martinez
Copy link
Owner

@c-martinez c-martinez commented Apr 24, 2024

I'm using this PR within my repo to have a code review before pushing upstream.

As far as I understand, the workflow should be that you create a package, and add a CFF as follows:

usethis::create_package('.')
usethis::use_cff()

And the output should be something like this:

#> ✔ Writing 'citation.cff'.
# ☐ Edit 'citation.cff'. (see <https://book.the-turing-way.org/communication/citable/citable-cffinit.html> and <https://citation-file-format.github.io/cff-initializer-javascript/#/>)

Things I am not very sure about:

  • Can we fill some information in the created CITATION.cff file? It looks to me as if some fields could be pre-filled for the user, for example license, author name, release date?
  • Are tests sufficient?
  • Is documentation sufficient?
  • Do we need to generate documentation via roxygen2 somehow? (see I saw man/*.Rd files mention not to edit by hand, but I have no idea how this works).

@c-martinez
Copy link
Owner Author

Invited @PabRod as collaborator, once he accepts, I can assign him as reviewer.

@PabRod
Copy link
Collaborator

PabRod commented Apr 25, 2024

I booked some time to take a look at this on Tue 30th.

@c-martinez c-martinez requested a review from PabRod April 25, 2024 07:43
NAMESPACE Outdated
@@ -106,6 +106,7 @@ export(use_cc0_license)
export(use_ccby_license)
export(use_circleci)
export(use_circleci_badge)
export(use_cff)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very good. Are you sure this is the first time you use R?

R/cff.R Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the parallel with use-citation() (available in R/citation.R). It looks good, but I am slightly confused about their not-so-straightforward workflow.

@PabRod
Copy link
Collaborator

PabRod commented Apr 30, 2024

@c-martinez:

  • Do we need to generate documentation via roxygen2 somehow? (see I saw man/*.Rd files mention not to edit by hand, but I have no idea how this works).

Documentation can be generated from the Build/More menu. In R jargon, this is know as roxygenizing.

It is usually a good idea to set it to roxygenize on every Install and restart from the Build/More/Configure build tools/Documentation with Roxygen/Configure... submenu.

Forgetting to roxygenize is a very common mistake. Indeed, there were functions in main that were not roxygenized.

@PabRod
Copy link
Collaborator

PabRod commented Apr 30, 2024

  • Can we fill some information in the created CITATION.cff file? It looks to me as if some fields could be pre-filled for the user, for example license, author name, release date?

I suggest keeping it as it is. I like how it mirrors use_citation(). If you disagree, please mark it again as to do.

@PabRod
Copy link
Collaborator

PabRod commented Apr 30, 2024

  • Are tests sufficient?

I'd say yes, but for some reason yours is not running. The whole battery of tests shows many fails, and it stops before reaching this one. I couldn't manage to run this test only, and I don't fully understand why. My guess: it is related with their workflow.

Update: I was wrong. After updating my R to the latest version, only one test fails. It's not the cff one!

@PabRod
Copy link
Collaborator

PabRod commented Apr 30, 2024

  • Is documentation sufficient?

It is definitely not insufficient, but let's:

  • Draft a pull request explaining why cff is important
    • and consider editing the in-code documentation afterwards

@c-martinez
Copy link
Owner Author

I didn't see you edited your comment about tests. It looks like you can run a single test: https://testthat.r-lib.org/reference/test_file.html but I somehow don't get that to work (clearly need to learn how to use R studio properly).

Anyway, if you are satisfied with this review, I will merge and draft a PR.

@c-martinez c-martinez merged commit fd9e538 into main May 1, 2024
@c-martinez
Copy link
Owner Author

I've created a PR, now let see if they have any feedback. Thanks for your help!

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