New hook: cff

[joelnitta]

trafficstars

Fixes #446.

When I run R CMD check (with devtools::check()), I get an ERROR and two NOTEs, but I believe the error is unrelated to this PR and only one of the notes is.

── R CMD check results ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── precommit 0.3.2.9004 ────
Duration: 2m 25.7s

❯ checking tests ...
  See below...

❯ checking package subdirectories ... NOTE
  Found the following CITATION file in a non-standard place:
    tests/testthat/in/CITATION.cff
  Most likely ‘inst/CITATION’ should be used instead.

❯ checking dependencies in R code ... NOTE
  Namespace in Imports field not imported from: ‘here’
    All declared Imports should be used.

── Test failures ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── testthat ────

> library(testthat)
> library(precommit)
> test_check("precommit")
/var/folders/gp/h66562zx3ps986y5z2jzrc0h0000gn/T/Rtmp2kLY0n/working_dir/RtmpQZI87R/file6eb740a264c5/.Rbuildignore
[ FAIL 1 | WARN 0 | SKIP 4 | PASS 234 ]

══ Skipped tests ═══════════════════════════════════════════════════════════════
• !is_windows() is TRUE (1)
• not_conda() is TRUE (3)

══ Failed tests ════════════════════════════════════════════════════════════════
── Failure ('test-conda.R:179'): fails gracefully when reticulate is not available ──
`install_precommit()` did not throw an error.

[ FAIL 1 | WARN 0 | SKIP 4 | PASS 234 ]
Error: Test failures
Execution halted

1 error ✖ | 0 warnings ✔ | 2 notes ✖

I used brew to install pre-commit, not conda; perhaps that has something to do with the error?

I didn't use {here} in any of my code. Perhaps it does need to be imported?

You may need to indicate in cran-comments.md that the note about location of CITATION.cff does not indicate a problem.

[lorenzwalthert]

Thanks @joelnitta, I will have a look at this next week. I think most notes, errors and warnings are as you said I related to your PR.

[lorenzwalthert]

As the error says, the hook must be listed in the .pre-commit-config.yaml located in the test resources to make the end-2-end test pass.

[lorenzwalthert]

I used brew to install pre-commit, not conda; perhaps that has something to do with the error?

Yeah, we should make the condo tests conditional on having condo installed. This currently works with not_conda(), that depends on an env variable. Better would be to have the following checks to determine the return value of not_conda():

• check if conda is on path. If no, return TRUE.
• if yes: check if pre-commit is installed on that path. If no, return TRUE,
• if yes: check if PRECOMMIT_INSTALLATION_METHOD is set to "conda". If no, return TRUE.
• if yes, return FALSE.

That way, testing should work better in local setup, i.e. not only on CI/CD where the env variables are set. I guess this would be a separate issue to fix (if you are interested, PR welcome 😀).

[lorenzwalthert]

Namespace in Imports field not imported from: ‘here’

This is annoying and I think a bug. here::here() is used as a default argument in some functions, but CRAN does not get that and think it's unused. Which is technically true but who wants a default argument that can't be executed?

[lorenzwalthert]

You may need to indicate in cran-comments.md that the note about location of CITATION.cff does not indicate a problem.

True. We have a similar problem for .Rprofile with some tests and save the reference file with the name Rprofile and then re-name it within the test (i.e. after moving it to the temp dir for testing).

  run_test("deps-in-desc",
    "Rprofile",
    suffix = "", std_err = "Dependency check failed",
    artifacts = c("DESCRIPTION" = test_path("in/DESCRIPTION")),
    file_transformer = function(files) {
      writeLines("R.cache::findCache", files)
      fs::file_move(
        files,
        fs::path(fs::path_dir(files), paste0(".", fs::path_file(files)))
      )
    }
  )

Can you do the same with your citation file?

[lorenzwalthert]

Thanks overall, very good PR for a first contributor 🥳

[lorenzwalthert]

Also, maybe once this gets merged, we can ask the {cff} maintainers to also document our hook (based on the pre-commit.com framework) as another backend for the pre-commit hook they have built on top of the bare git hook functionality?

[joelnitta]

As the error says, the hook must be listed in the .pre-commit-config.yaml located in the test resources to make the end-2-end test pass.

Yes, I missed that -- but I see in .pre-commit-config.yaml that id: codemeta-description-updated is commented out. Should this also be commented out? If so, adding it (obviously) won't change anything?

[lorenzwalthert]

Yes, I missed that -- but I see in .pre-commit-config.yaml that id: codemeta-description-updated is commented out.

In the root it’s commented out. But there is a sample config file testthat/resources or similar that need to have the hook listed. That’s just for testing. See also the workflow file of the failing GitHub Action in .github/

[lorenzwalthert]

Did you see my other comments (not liked to lines of code) above?

[joelnitta]

Did you see my other comments (not liked to lines of code) above?

Yes (thanks!) but I'm going to be traveling soon and may not get to this for a while, so I thought I'd send what I've fixed so far. Not much but it's a start.

[lorenzwalthert]

@joelnitta do you want to finish this up or should we close it?

[joelnitta]

Sorry, fell off the radar. Thanks for the reminder, I'll try to get to this in the next week, or you can close it.

[joelnitta]

@lorenzwalthert would you consider updating to the most recent renv? I am having trouble re-creating the project library, since I am coming back to this after a while and am needing to update packages. Specifically I'm having problems with {digest}, which I can install using renv v1.0.3 but not v0.17.3.

[lorenzwalthert]

I suggest you update your fork, because the digest dependency was removed. If that does not solve the problem, delete the renv/ folder and then start over.

[joelnitta]

the digest dependency was removed

Even though it is not listed in Imports or Suggests, it is still present in renv.lock in main:

https://github.com/lorenzwalthert/precommit/blob/e989416a1ed7f7cee43b4e5a0d7ff4c234dc4b1d/renv.lock#L199