lorenzwalthert / precommit

pre-commit hooks for R projects
https://lorenzwalthert.github.io/precommit/
GNU General Public License v3.0
246 stars 46 forks source link

New hook: cff #446

Open joelnitta opened 1 year ago

joelnitta commented 1 year ago

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.

lorenzwalthert commented 1 year ago

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 commented 1 year ago

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.

joelnitta commented 1 year ago

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

lorenzwalthert commented 1 year ago

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

joelnitta commented 1 year ago

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 commented 1 year ago

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.

lorenzwalthert commented 1 year ago

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 commented 1 year ago

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.

joelnitta commented 1 year ago

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?

lorenzwalthert commented 1 year ago

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.

joelnitta commented 1 year ago

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...

lorenzwalthert commented 1 year ago

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:
  - lintr@3.0.1
lorenzwalthert commented 1 year ago

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

joelnitta commented 1 year ago

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

lorenzwalthert commented 1 year ago

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