Add minimal pre-commit config #150
Conversation
| - repo: https://github.com/lorenzwalthert/precommit | ||
| rev: v0.4.2 | ||
| hooks: | ||
| - id: style-files |
There was a problem hiding this comment.
This one can be quite annoying, to be honest. It often makes changes unrelated to the PR.
There was a problem hiding this comment.
The way I understand stylers/linters like this is that making changes unrelated to the PR only happens if the style is not upheld to begin with. This is a common thing with for example prettier in JavaScript too. Is the annoyance with the styler or with the style itself?
This will not happen so much with new instances of packagetemplate inasmuch for existing packages. For new instances, the pre-commit hook ensures the style is applied from the beginning, hence, it will only make changes where the PR is doing things anyway. In older packages, if style is not consistent, it would cause changes upon first run to get everything in style. We could opt to run the styler before setting up the pre-commit hook to ensure PRs don't get clogged unnecessarily - which is a valid point by @jamesmbaazam 👍
There was a problem hiding this comment.
We could opt to run the styler before setting up the pre-commit hook to ensure PRs don't get clogged unnecessarily - which is a valid point by @jamesmbaazam 👍
Yes, that's a good workaround. In the past, I've not included the style changes to a PR if it's not within its scope as it can be distracting to review.
There was a problem hiding this comment.
I love the idea of a pre-commit hook - I rely on these a lot in NodeJS development (there I would also run the tests before committing). I am in favor of using something like this in principle and by default, if it is low maintenance.
I have not run pre-commit hooks in R before and I took this opportunity to try and run the packagetemplate pre-commit hook. Honestly, it was not obvious that I needed to do additional setup to get them to work.
After some digging I found some docs but they were not complete.
Specifically, the steps I had to take to successfully run these commit hooks is included in the details below. It took me well over an hour to get this set up, which is longer than I expected this task to take.
brew install pre-commitinstall.packages('precommit')
install.packages('rstudioapi')
precommit::use_precommit()
# visit pre-commit.ci and login with github
# don't know whether this step was truly necessary as it kept popping up after
# • It seems like you are using the roxygenize hook. This requires further edits
# in your `.pre-commit-config.yaml`, please run
# `precommit::snippet_generate('additional-deps-roxygenize')` to proceed.
precommit::snippet_generate('additional-deps-roxygenize')
# requires desc
install.packages('desc')
# run again
precommit::snippet_generate('additional-deps-roxygenize')
install.packages('pkgdown')
install.packages('pak')
pak::pak('epiverse-trace/epiversetheme')Some of these steps may or may not be needed depending on the packages already installed, of course.
I also learned that git hooks are not copied upon cloning a repo and we need some procedure to set it up for the person, which is what precommit is doing.
Currently I am unsure whether this is really all that setup friendly and whether it will get used as a result. It is not easy enough to make it the default behavior, and so having it as opt-in is good in that scenario. I am slightly hesitant but okay to approve this as it is opt-in and does not force any standards upon anyone.
If we implement this, it will be good to do a skillshare on this in any case for awareness and giving people what they need to make a decision on (not) using it.
| - repo: https://github.com/lorenzwalthert/precommit | ||
| rev: v0.4.2 | ||
| hooks: | ||
| - id: style-files |
There was a problem hiding this comment.
The way I understand stylers/linters like this is that making changes unrelated to the PR only happens if the style is not upheld to begin with. This is a common thing with for example prettier in JavaScript too. Is the annoyance with the styler or with the style itself?
This will not happen so much with new instances of packagetemplate inasmuch for existing packages. For new instances, the pre-commit hook ensures the style is applied from the beginning, hence, it will only make changes where the PR is doing things anyway. In older packages, if style is not consistent, it would cause changes upon first run to get everything in style. We could opt to run the styler before setting up the pre-commit hook to ensure PRs don't get clogged unnecessarily - which is a valid point by @jamesmbaazam 👍
|
Thanks both for the feedback! Merging as it is indeed opt-in and will not cause any disruption to anyone who doesn't wish to use them, but they still answer a frequently expressed need. I agree a skillsharing session on this would be great and I have shared a video tutorial in the resource library in the meantime. |
NEWS.mdFix epiverse-trace/blueprints#12. This can be paired with a submission of the related UseR2022! video to the resource library.
As mentioned in the changelog, installation of the pre-commit tool, and explicit opt-in of individual developers is still required.