Fix r hooks when hook repo is a package #1831
Conversation
pre_commit/languages/r.py
Outdated
| cmd_output_b( | ||
| 'Rscript', '--vanilla', '-e', | ||
| """\ | ||
| "prefix_dir <- '" + prefix.prefix_dir + "'" + |
There was a problem hiding this comment.
this should use an f-string to do formatting, also triple-quoted literals can contain quote characters
the backslash was intentional, actually -- it makes the line numbers of the string match up with the string (and avoids a blank line at the top of your string)
There was a problem hiding this comment.
Thanks, makes all sense. Was too lazy to check if python < 3.6 is required to make f strings work... should be fixed now. Also, because the prefix_dir is passed into string expression (Rscript -e {expr}), another escaping is required for Windows, as it has backward slashes in the path. Maybe there is a better way to do this but I could not find one... Hope it does not create other unexpected problems.
de3bca2 to
f73fb10
Compare
There was a problem hiding this comment.
otherwise looks good -- just a small thing
pre_commit/languages/r.py
Outdated
| prefix_dir <- "{ | ||
| prefix.prefix_dir.encode('unicode_escape').decode() | ||
| }" |
There was a problem hiding this comment.
I think you can use {prefix.prefix_dir!r} to do this as well -- that'll give you the quotes and then you won't have to do an encoding dance
There was a problem hiding this comment.
Nice. I really start to like Python string manipulations 😀. All checks pass now.
In #1799, we assume
env_dirinstead ofprefix_diras the package source and install it, which creates a package with the right name (because ofDESCRIPTION), but obviously does not expose any API and is hence useless. Hooks that rely on the installed package hence fail.Also removing an (apparently) randomly introduced\that does not cause any harm.