precommit CI observations

[pat-s]

trafficstars

• It seems that the precommit CI is styling the full package and not only the changed files. Is this indented?
• Given that there is never a cache on CI systems, the verbose warnings for the missing styler cache could be turned off? (already mentioned in #334)
• Even on a small project of mine which does not use tidyr or dplyr I got the following additional_dependencies request (I can't see why):

    -   id: roxygenize
        additional_dependencies:
        -    tidyr
        -    dplyr

• Spell check hook also catches .png file paths?

There were 50 or more warnings (use warnings() to see the first 50)
The following spelling errors were found:
  WORD               FOUND IN
ABW                plot_gam3.png:272
AE                 plot_gam3.png:80
[...]

• I think for users to not get frustrated when they try out precommit only stable hooks should be active by default for now so that they have a pleasant onboarding experience - otherwise they might turn it off again and not revisit - what do you think?

Happy to help making the precommit CI experience even better 🚀

[lorenzwalthert]

It seems that the precommit CI is styling the full package and not only the changed files. Is this indented?

It does not seem that pre-commit.ci supports running on the diff of the files. Maybe this could be requested?

Even on a small project of mine which does not use tidyr or dplyr I got the following additional_dependencies request (I can't see why):

These were examples (if you read the error message carefully), but maybe they could be replaced with more obvious examples like example-gh-org/example.repo or eXamplePackage.

I think for users to not get frustrated when they try out precommit only stable hooks should be active by default for now so that they have a pleasant onboarding experience - otherwise they might turn it off again and not revisit - what do you think?

I agree. Are you talking about hooks to be skipped in CI or also locally?

[lorenzwalthert]

.jpeg are excluded in recent templates.

[lorenzwalthert]

other consideration is to move the roxygenize hook back to language: script -.-

[pat-s]

These were examples (if you read the error message carefully), but maybe they could be replaced with more obvious examples like example-gh-org/example.repo or eXamplePackage.

Ah ups!

I agree. Are you talking about hooks to be skipped in CI or also locally?

For now only in CI? Installing these deps locally might not be that troublesome. Though the issue which just comes to mind right now is that each repo would then have a unique pre-commit.yaml and c/p (or using a template) because of the unique dependencies listed. I guess you already evaluated whether an explicit definition in the YAML is really required or if the deps could be installed silently in the background?

[lorenzwalthert]

I guess you already evaluated whether an explicit definition in the YAML is really required or if the deps could be installed silently in the background?

Yes, explicit listing is required (as long as we don't depend on global R library via language: script). Pre-commit was not built for hooks that require the local directory to be installed in the virtual environment, but for generic, small checks like formatting and similar. Also, users could override the language in their .pre-commit-config.yaml I think.

[lorenzwalthert]

pre-commit may ultimately support running on diff only: https://github.com/pre-commit-ci/issues/issues/90

[pat-s]

The roxygenize hook will fail if certain packages depend on local system libs, as e.g. terra does: https://results.pre-commit.ci/run/github/233107318/1640161134.lTNczNlqS4CY74DzLIRQlA.

[lorenzwalthert]

Yes. System libraries are not supported with pre-commit.ci as explained in the vignette: https://lorenzwalthert.github.io/precommit/articles/ci.html This is all because roxygen requires pkgload requires system dependency. 😩

that’s maybe why we should skip that hook by default on ci.

[pat-s]

that’s maybe why we should skip that hook by default on ci.

Probably a good idea, yes. Maybe combined with a note that points to the vignette which explains this default.

[pat-s]

Today I observed a strange cache error: https://results.pre-commit.ci/run/github/296362546/1650700767.C0uTqUxrTvCWUsev2ln5LA

Error: statfs /home/pcworker/.cache/pre-commit-ci/lang/01a20d4562735887d3363aab0868e1baad7df919585c0843e2d3f3638a54adc2/r: permission denied

config: https://github.com/cynkra/cynkrathis/blob/main/.pre-commit-config.yaml

[lorenzwalthert]

@pat-s does the problem persist?

[pat-s]

Haven't seen this one since then I guess.