Contribute a git hook to pre-commit.com

[katrinleinweber]

Update

A broader solution than discusses here can be found in lorenzwalthert/pre-commit-hooks.

Original post

I read in #452 that git integration is not a priority, but maybe a small hook that just styles the files that one has git added / staged would be a minimal, viable step into that direction. Maybe as a contribution to pre-commit.com?

Do you think it would be possible to shell-script styler in that way? I played around a bit, but ran into problems.

[katrinleinweber]

for f in $(git diff --name-only --cached | grep '.R')
do
	Rscript -e "styler::style_file($f)"
done

yields Error in dirname(path) : object 'R' not found. Probably, because the files are of course in an R/ subdirectory.

[lorenzwalthert]

Hi @katrinleinweber, thanks.

I read in #452 that git integration is not a priority

Thanks for considering old issues. I did not mean I am strictly against it, but I at least won't develop it myself. If I understand correctly, the git hock would not be implemented in the source code of styler, right? I think it's a cool thing. I am not sure but I believe you could simply defer the path expansion to R?

for f in $(git diff --name-only --cached | grep '.R')
do
	Rscript -e "styler::style_file(file.path('R', $f))"
done

It is unfortunate that pre-commit.com does not support R because the above script might have problems due portability across operation systems. Are you on Windows? I can try the script on my Mac. Maybe we can add support for R to pre-commit.com too, although that might be more work?

[katrinleinweber]

Thanks for contextualising the idea! Yes, that would certainly be more work.

the git hook would not be implemented in the source code of styler

I guessed so (over in pre-commit/pre-commit-hooks), but pre-commit's contributing docu says: "get the .pre-commit-hooks.yaml files added to popular linters". I guess that means styler as well ;-) OTOH, those files are present in so many more, normal projects.

Hm... I'll inquire about the process of adding R support on their end.

[katrinleinweber]

Apparently, they need an r.py file that acts as a module handling the hook installation & run within their Python package.

[asottile]

Exciting! Yeah the work to make this happen would be in two parts:

1. Implement r as a language in pre-commit
2. Add a .pre-commit-hooks.yaml file in this repository

Does styler provide an executable when installed? and/or what's the usual interface that people run tools written in R?

[lorenzwalthert]

If you mean a bash-like command when you say executable, the answer is no. I don't know of any R package that does so to be honest. In almost all cases, people execute tools written in R in the R console (e.g. when using RStudio) or they invoke R from the command line with Rscript -e "styler::style_file(...)".

[lorenzwalthert]

References

• Article about adding a pre-commit hook for scala source code formatter.
• https://ljvmiranda921.github.io/notebook/2018/06/21/precommits-using-black-and-flake8/
• Some example hocks: https://github.com/pre-commit/pre-commit-hooks

[michaelquinn32]

Pre-commits seem better placed somewhere like usethis, which contains development tools that wrap styler. Plus, that package already sets up a user's git for a project.

[bfgray3]

I'd like to add my vote in support of a pre-commit hook.

[jackwasey]

A less invasive approach would be simply to report whether files would be styled, thus preventing a commit. Since it is possible that styler could generate invalid code, at least some people would prefer a message, like codemeta gives, instead of doing it for you. It is also a slow process for a bigger package, although checking whether styling is needed is probably just as slow as actually performing it, until there is some caching in styler.

[lorenzwalthert]

Also note this reference: #452.

[lorenzwalthert]

@jackwasey indeed that would be less invasive. Indeed a style_pkg() call as a pre-commit hook would be is quite slow currently, we have implemented neither multi-processing and caching. Please refer to #370 and #491 and #320.

Edit:* Of course, we'd only style the files changed, not the whole package, as drafted above.

[lorenzwalthert]

For all people who can't wait for a git hook, have a look at the starters package, they seem to have implemented such a hook for styler: lockedata/starters#138.

cc: @jonmcalder if you are interested in contributing here, most welcome. We also discussed how to add R as a language for pre-commit.com (pre-commit/pre-commit#926), but it seems to quite some work/overhead.

[lorenzwalthert]

Pre-commits seem better placed somewhere like usethis, which contains development tools that wrap styler. Plus, that package already sets up a user's git for a project.

I think that's a fair point. There might be other pre-commits one would like to implement, not just for styler. Ideally we'd add R as a language to pre-commit.com or a similar framework. Feel free to open an issue in usethis for this.

[katrinleinweber]

… how to add R as a language for pre-commit.com

An alternative might be https://github.com/sds/overcommit?

Pre-commits seem better placed somewhere like usethis, which contains development tools that wrap styler. Plus, that package already sets up a user's git for a project.

True, but is usethis also relevant for non-package projects? Git & Styler are, IMHO.

Plus, I generally think that integrating what we want into an upstream framework is more fruitful in the long run than integrating it into R-specific tooling only.

[lorenzwalthert]

True, but is usethis also relevant for non-package projects?

Yes. Maybe it is worth looking into infrastructure that usethis has for git hooks, like use_git_hook(). This you could use if you want to manually define how to add a pre-commit hook, so it is not the same thing as we are trying here because with a framework like pre-commit.com, the end user would not specify the code, just the name of the hook, because we would implement it. Is that correct?

To me, it does not really matter to which framework we contribute it. What are the advantages of overcommit @katrinleinweber? Also, obviously these are not exclusive. We can implement it in both eventually if people want this. As of now, I think using sys as a language and entry: Rscript styler::style_*(...) for pre-commit.com would be not so difficult.

[lorenzwalthert]

Bottom line: It worked!

Now, just create a .pre-commit-config.yaml in your project (if you don't have one already) and add this to it if you are willing to accept that this is very experimental:

-   repo: https://github.com/lorenzwalthert/styler_precommit
    rev: b8af4e51fb273fdf57887030acdbfd8fe678cec9  # Use the ref you want to point at
    hooks:
    -   id: style-files

In full length:

I just did some more research on pre-commit.com and I think I now understand the crux. For python pre-commit hooks, it says in the documentation:

The hook repository must have a setup.py. It will be installed via pip install .. The installed package will provide an executable that will match the entry – usually through console_scripts or scripts in setup.py.

This is exactly what we see in the pre-commit/pre_commit_hoocks: https://github.com/pre-commit/pre-commit-hooks/blob/master/setup.cfg. The python functions are exposed as command line executables. In R, there is no way we could create a command line executable from an R package. This was apparently a deliberate design decision (https://stackoverflow.com/questions/44566100/installing-executable-scripts-with-r-package).

Anyways, because pre-commit.com does not yet support R and there is no way we could create the necessary executable anyways, I just decided to take the ugly approach for an experiment: Create a python pre-commit that calls a subprocess that calls R. It is worth noticing that all these things are triggered by git commit, so the system calls the hook, which executes python code (because that is what pre-commit is written in) which calls python which calls the system which calls R. After fiddling for some time, it worked!

This is very experimental! We do not guarantee this to continue to work. We need to find a better way and find out 1) where to best host the python package, 2) how to name the executable 3) error handling 4) do testing, documentation and other things.

If you want to give it a try, most welcomed.

[asottile]

Neat! One thing that you could do is use language: script instead (and then you don't need a python package)

Then you could have an entrypoint like:

#!/usr/bin/env python

# (same contents as your current script)

And then a config like:

-   id: ...
    language: script
    entry: ./bin/run-r-styler

[lorenzwalthert]

[...] Neat!

Not sure, I think it's a bit hacky 😄. But thanks for looking into it @asottile. Yes, I was wondering about using language: script but I could not easily find an example so I just decided to go for language: python for a first pass. So if we choose to use language: script, how would that affect my repo structure? Could my repo at https://github.com/lorenzwalthert/styler_precommit then just contain a single script run-r-styler (with shebang, as you suggested above) instead of a whole package? Because I don't fully understand this part of the documentation and how this relates to your example above:

Script hooks provide a way to write simple scripts which validate files. The entry should be a path relative to the root of the hook repository.

Maybe then I don't even have to use python in my code because with R, we can use Rscript with shebang, so could I have

# !/usr/bin/env Rscript

# R code only

Correct?
Since you are in this thread, are there any recommendations regarding naming conventions of repositories and hook ids you'd suggest to use for styler so we can make it match conventions of pre-commit for other linters and style fixers? I saw there is autopep8 and flake support, so maybe check out those?

An advantage of using the python package could be that we'd be able to expose a command line executable for styler that could be used independently of pre-commit, which is not possible with an R package. Related: #494.

[asottile]

Maybe then I don't even have to use python in my code because with R, we can use Rscript with shebang, so could I have

# !/usr/bin/env Rscript

# R code only

Correct?

Yep! That would be even simpler :D

Since you are in this thread, are there any recommendations regarding naming conventions of repositories and hook ids you'd suggest to use for styler so we can make it match conventions of pre-commit for other linters and style fixers? I saw there is autopep8 and flake support, so maybe check out those?

There isn't currently any sort of naming convention -- though they can be added to our list of repositories on the website via this -- ends up here.

It's ~generally preferred to have it in the actual repo of the tool -- but in this case it's probably better separated since the versioning won't properly lock (which is what is ~mostly expected of other repos)

An advantage of using the python package could be that we'd be able to expose a command line executable for styler that could be used independently of pre-commit, which is not possible with an R package

Ah neat, hadn't even thought about that -- it's really surprising to me that R packages can't have commandline tools :S

[katrinleinweber]

1k thanks for getting this up and running :-) I followed #467 (comment) just now and it worked fine and committed the style-modifications.

What are the advantages of overcommit @katrinleinweber?

If I had noticed any clearly relevant one, I would have written so initially ;-) "It's in Ruby" could be considered an advantage, but since academia choose Python and most R folks are more familiar with that, I didn't consider it relevant. Just a note for completeness sake.

[lorenzwalthert]

@asottile I think when using language: script, another disadvantage is that the user has to manually make the script executable and the directory where it lays is something like ~/.cache/pre-commit/repof4lOTz/bin/executable, so it's hard to even give the user clear instructions when they are not experienced command line users. At least in my case that seemed necessary. Any workaround for this?
Solution: Just make the file executable and commit because the permission is an attribute of the file.

[asottile]

You should be able to check it in executable chmod +x script && git add script

[lorenzwalthert]

Summary:

Put a .pre-commit-config.yaml in your repo root with this content:

-   repo: https://github.com/lorenzwalthert/pre-commit-hooks
    rev: 2bff9322dfab68f08e4c722c6caee7800e966b2a
    hooks: 
    -   id: styler-style-files

And you are all set. There is another hook for devtools::document() and you are welcomed to contribute others. See https://github.com/lorenzwalthert/pre-commit-hooks.

Edit: For this to work correctly in all cases (see below), you need the dev version of styler or the CRAN release after version 1.1.1.

[lorenzwalthert]

Need to tweet / communicate this before closing.

[katrinleinweber]

Thank you for building this @lorenzwalthert & @asottile :-)

[asottile]

no problem! happy to help :D -- @lorenzwalthert did most of the heavy lifting to make it happen -- I just assisted :)

[lorenzwalthert]

In case you were using the hook: There was a bug in styler (#521) that made it impossible to run the hook correctly on R files in different directories. It is fixed in #522 so if you want this to work correctly, get the dev version of styler.