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

Add renv.lock validation hook #558

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jrdnbradford
Copy link

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

@lorenzwalthert
Copy link
Owner

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

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

@jrdnbradford
Copy link
Author

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.

@lorenzwalthert
Copy link
Owner

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.

@jrdnbradford
Copy link
Author

My renv::lockfile_validate() function was merged over at rstudio/renv#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.

@lorenzwalthert
Copy link
Owner

lorenzwalthert commented Jun 20, 2024

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

@jrdnbradford
Copy link
Author

Working on this now!

@jrdnbradford
Copy link
Author

@lorenzwalthert I think I've got this figured out. Happy to take suggestions for improvements! 🚀

<!-- -->

id: renv-lockfile-validate
args: [--schema=<schema>] [--greedy --error --verbose --strict]
Copy link
Owner

@lorenzwalthert lorenzwalthert Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please link (with hyper-text-link) to docs for env::lockfile_validate() since you just pass arguments there?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe let's wait for rstudio/renv#2048?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

id: renv-lockfile-validate
args: [--error]

This hook does not modify files.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add the version of {precommit} this hook was added to (similar to other hooks)?

@jrdnbradford
Copy link
Author

jrdnbradford commented Dec 15, 2024

As discussed in conversations above, I'll wait till the renv docs issue is resolved before bumping the precommit dev version and making the recommended updates the docs.

I pushed some changes that should resolve most, maybe all, of the GHA tests.

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 this pull request may close these issues.

2 participants