precommit icon indicating copy to clipboard operation
precommit copied to clipboard

Add `renv.lock` validation hook

Open jrdnbradford opened this issue 1 year ago • 6 comments
trafficstars

Any interest in adding a hook like this?

When used correctly, you should never really have to worry that your renv.lock file does not fit the schema. But in practice, especially with collaborators, you may want an extra check that no one has accidentally edited fields as this can cause difficult to diagnose errors. Never hurts to have an automated check!

{renv} doesn't actually provide a schema, but I believe I've provided one that fits it. It may need to go into its own file instead of being a string, but I couldn't get the tests to pass when I had it in inst/.

jrdnbradford avatar Apr 16 '24 02:04 jrdnbradford

Interesting. However, I think I'd only want to support this hook if {renv} officially provided a schema. Did you talk to Kevin about it or open an issue for it?

lorenzwalthert avatar Apr 16 '24 16:04 lorenzwalthert

Also, please note that before submitting new PRs, discussion in an issue is encouraged as per our CONTRIBUTING.md.

lorenzwalthert avatar Apr 16 '24 16:04 lorenzwalthert

Ah, apologies, embarrassing that I missed that.

Looks like this is the current state of what the {renv} maintainers think about having a schema:

The renv.lock lockfile is a JSON file, and while no schema is provided, you should be able to infer the structure from the existing fields.

I can open an issue to see if there's any interest. If not, I/you can close this PR.

jrdnbradford avatar Apr 16 '24 17:04 jrdnbradford

Yes, please open one and see what happens. I would like to offload the task of updating the schema upstream as well as how to deal with an evolving schema. These are core {renv} functionalities. As a strategy of last resort, you can also implement this hook in your own hook repo instead of here. It’s not too much work.

lorenzwalthert avatar Apr 17 '24 05:04 lorenzwalthert

My renv::lockfile_validate() function was merged over at https://github.com/rstudio/renv/pull/1889. If you have interest in a pre-commit hook that utilizes the functionality, I'd be happy to put in a new PR, let me know. Either way probably best to close this PR (#558) and put in a new one if you want the hook.

jrdnbradford avatar Jun 20 '24 19:06 jrdnbradford

Glad to hear that you got this merged upstream. ~Yes, please open a new PR~. Please force push your changes to this PR.

lorenzwalthert avatar Jun 20 '24 22:06 lorenzwalthert