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

New hook: cff #446

Open
joelnitta opened this issue Nov 10, 2022 · 15 comments · May be fixed by #456
Open

New hook: cff #446

joelnitta opened this issue Nov 10, 2022 · 15 comments · May be fixed by #456

Comments

@joelnitta
Copy link

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

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

@lorenzwalthert
Copy link
Owner

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

@joelnitta
Copy link
Author

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}:

if (legacy_hooks == "remove") "--overwrite",

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
Copy link
Author

joelnitta commented Nov 12, 2022

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.

rlang::abort("codemeta.json is out of date; please re-run codemetar::write_codemeta().")

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

joelnitta commented Nov 15, 2022

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
Copy link
Owner

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
Copy link
Author

Thanks. Actually the reason I ask is because of a different problem: #451

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

@lorenzwalthert
Copy link
Owner

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
Copy link
Owner

lorenzwalthert commented Dec 6, 2022

@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
Copy link
Author

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

@lorenzwalthert
Copy link
Owner

lorenzwalthert commented Dec 6, 2022

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

@joelnitta joelnitta linked a pull request Dec 8, 2022 that will close this issue
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 a pull request may close this issue.

2 participants