R local hooks should not prefix hook path#1878
Conversation
|
is this an actual problem? the |
|
Starting point was one of my repos where I got the error: Here is an example repo. With So I thought there is a path problem. I tried to understand how the prefix was set and arrived here: def _prefix(language_name: str, deps: Sequence[str]) -> Prefix:
language = languages[language_name]
# pygrep / script / system / docker_image do not have
# environments so they work out of the current directory
if language.ENVIRONMENT_DIR is None:
return Prefix(os.getcwd())
else:
return Prefix(store.make_local(deps))This made me believe understanding why it works for script hooks but not for r hooks. Because one has an env and the other one does not. |
|
oh hmmmm -- right for language'd hooks it does go through a different path. that's trickier then I'll have to think about this some more |
|
hmmm ok so how about this idea -- what if there were a separate |
If you mean in the sense as I am not sure we could get rid of that problem with a new language that way without still relying on PS: To better understand pre-commit, I also tried to compare to the python language and realised there is also one combination of local/remote and path/expression that does not work (but one can circumvent it in python): python hooks (prefix is not prepended to path in run_hook):
script hooks (prefix is prepended to path in run_hook):
R cannot expose CLI tools like python: -> we can only use scripts and hence not use that work the around. So it feels like we want the best of 1 I am assuming this new language |
|
ah ok, when I mentioned the thing about in that case, it's probably ok to make |
|
Thanks Anthony. Then let's progress with |
|
happy to spend time on this -- I think R is a valuable addition to the ecosystem (despite it being weird!) |
|
oops, this conflicts with the other changes |
f972bc9 to
466694e
Compare
|
... which I caused 🙃. Fixed. |

The
entryfor r hooks is very similar tolanguage: script(in the case a path is supplied):For local hooks, we don't need to prefix the path (which currently happens). This PR fixes this if
scrof thehookis local.