precommit icon indicating copy to clipboard operation
precommit copied to clipboard

New hook: cff

Open joelnitta opened this issue 3 years ago • 15 comments

Similar to codemeta.json, CITATION.cff provides machine and human-readable information about how to cite a package. The cffr package can auto-generate it, and already has a hook available. It would be great if this hook was included in precommit.

joelnitta avatar Nov 10 '22 01:11 joelnitta

Thanks for the idea @joelnitta. The thing is that I try to keep the depenency graph of {precommit} light, and {V8} as a recursive dependency does not meet this criteria for me. However, anyone can create their own git repo into a hooks repo by following the docs. For R, I think this repo might be the only one, but for most other languages, there are hundreds of repos that contain hook definitions.

lorenzwalthert avatar Nov 10 '22 08:11 lorenzwalthert

You can run pre-commit hooks from the pre-commit framework with other hooks alongside in the so-called migration mode, so there should be no compatibility issue.

lorenzwalthert avatar Nov 10 '22 08:11 lorenzwalthert

From the point of view of the {precommit} R package, would that be the equivalent of use_precommit(legacy_hooks = "allow")?

joelnitta avatar Nov 11 '22 07:11 joelnitta

I think so. Please consult upstream docs at pre-commit.com.

lorenzwalthert avatar Nov 11 '22 09:11 lorenzwalthert

This does seem to be the case. Upstream docs say

by default, if you have existing hooks pre-commit install will install in a migration mode which runs both your existing hooks and hooks for pre-commit. To disable this behavior, pass -f / --overwrite to the install command"

and this line controls said behavior from {precommit}:

https://github.com/lorenzwalthert/precommit/blob/3822108d8c1a3eb23b8266c5744f5e0078a72562/R/install.R#L84

I can confirm that first installing the {cffr} precommit hook with cffr::cff_git_hook_install(), then setting up precommit with precommit::use_precommit(legacy_hooks = "allow") does work so that both hooks are used (i.e., there are now two files in .git/hooks, pre-commit and pre-commit.legacy).

I am not sure how this would work if I wanted to add another pre-commit hook outside of the precommit framework, but it works for this particular case (precommit framework plus one more hook).

joelnitta avatar Nov 12 '22 00:11 joelnitta

Also @lorenzwalthert do you mind explaining why having the option to include the ccfr pre-commit hook would add cffr's dependencies? I think it could be done similarly to {codemeta}, where the hook just instructs the user to run codemetar::write_codemeta() and does not add {codemeta} to DEPENDS or SUGGESTS.

https://github.com/lorenzwalthert/precommit/blob/44a855ef5ca8a2c5703c44ea5bb12576a7327ceb/inst/hooks/exported/codemeta-description-updated.R#L31

I realize now this is different from using the available {cffr} hook as mentioned in my OP, but I think it would be straightforward to implement a hook in {precommit} that basically does the same thing as the codemeta hook, but for CITATION.cff instead of codemeta.json. If you agree I could work on a PR.

joelnitta avatar Nov 12 '22 00:11 joelnitta

I am not sure how this would work if I wanted to add another pre-commit hook outside of the precommit framework, but it works for this particular case (precommit framework plus one more hook).

that should work without issues.

lorenzwalthert avatar Nov 12 '22 12:11 lorenzwalthert

Also @lorenzwalthert do you mind explaining why having the option to include the ccfr pre-commit hook would add cffr's dependencies?

Short anser: If it's merely a check for outdated files and has no calls to {cffr}, you are right that it would not expand {precommit}'s dependency graph. In that case, I am happy to add a hook (and thanks for keep digging :D).

If you want to call {cffr} in the hook, here's what you should know: There are two types of dependencies. The ones relevant for running the UI functions of {precommit}, and the ones running the hooks. R is implemented as a second level language: It uses your computers R installation, but creates a separate virtual environment (with {renv}, hosted outside of your repos directory in a pre-commit system directory) for each hook repo (like lorenzwalthert/precommit). In that {renv}, the dependencies of the UI functions are not needed, only the dependencies of the hooks.

lorenzwalthert avatar Nov 12 '22 15:11 lorenzwalthert

creates a separate virtual environment (with {renv}, hosted outside of your repos directory in a pre-commit system directory)

@lorenzwalthert where is this system directory?

joelnitta avatar Nov 15 '22 07:11 joelnitta

I think ~/.cache/pre-commit/ on Unix contains all hook repos, one is the one for lorenzwalthert/precommit hooks that contains a {renv} somehow nested. Make your life easier with pre-commit clean your remove all and then install only the ones from lorenzwalthert/precommit, then there should be only one. But it’s not advised to manually edit these files.

lorenzwalthert avatar Nov 15 '22 07:11 lorenzwalthert

Thanks. Actually the reason I ask is because of a different problem: https://github.com/lorenzwalthert/precommit/issues/451

I thought I may try to update the renv lock file myself but having troubles...

joelnitta avatar Nov 15 '22 07:11 joelnitta

Well if you want to override dependencies specified in renv.lock from lorenzwalthert/precommit, just use additional_dependencies in .pre-commit-config.yaml:

- id: lintr
  additional_dependencies:
  - [email protected]

lorenzwalthert avatar Nov 15 '22 08:11 lorenzwalthert

@joelnitta do you want to implement the {cff} hook in the spirit of the {codemeta} hook? I won't have time to do that.

lorenzwalthert avatar Dec 06 '22 11:12 lorenzwalthert

Yes, I should be able to. I'll try to get to it this week.

joelnitta avatar Dec 06 '22 11:12 joelnitta

Great. You can also see other PRs that added hooks, e.g. the recent #393 or maybe more relevant the #350.

lorenzwalthert avatar Dec 06 '22 11:12 lorenzwalthert