Repository: pre-commit/pre-commit
Pull request #1878 (2021-04-15 08:59:38)

R local hooks should not prefix hook path
The `entry` for r hooks is very similar to `language: script` (in the case a path is supplied):

```yaml
-   repo: local
    hooks:
    -   id: consistent-release-tag
        name: consistent-release-tag
        entry: Rscript inst/consistent-release-tag
        language: r
```

For local hooks, we don't need to prefix the path (which currently happens). This PR fixes this if `scr` of the `hook` is local.

asottile: `hook.src` is only meant for error messages so I don't really want to add a dependency on it

is this an actual problem? the `prefix` should be the working directory for local hooks so it should be joining to the right place anyway (just making an absolute path)? -- this is consistent with how `script` hooks work
lorenzwalthert: Starting point was one of my repos where I got the error: Here is an [example repo](https://github.com/lorenzwalthert/pre-commit.test-renv-local). With `entry: Rscript inst/hook2`, I get

```
git commit -m 'more reasonable location for hook'              
Require bullet in NEWS.md................................................Failed
- hook id: require-news-on-push
- exit code: 2

Fatal error: cannot open file '/Users/lorenz/.cache/pre-commit/repoz2lpgpug/inst/hook2': No such file or directory
```

So I thought there is a path problem. I tried to understand how the prefix was set and arrived [here](https://github.com/pre-commit/pre-commit/blob/dbd69af90daba11e719e501c4d17ed710c13110d/pre_commit/repository.py#L149-L152):

```python
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. 
asottile: 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
asottile: hmmm ok so how about this idea -- what if there were a separate `rscript` and `r` language?  or does `PATH` lookup not make a lot of sense for R in general ?
lorenzwalthert: > or does PATH lookup not make a lot of sense for R in general ?

If you mean in the sense as `venv` and `conda` add their own python executable to `PATH`: No, this is not an option here because {renv} does not boostrap base R itself, only (addon) packages. I.e. it only modifies the path on which packages are loaded from *inside* R.

I am not sure we could get rid of that problem with a new language that way without still relying on `hook.scr`.<sup>[1](#myfootnote1)</sup> In `run_hook`, we must amend the script path based on whether or not the hook is local and I believe we can't solve it over `hook.prefix` (as it is solved for `language: script` in [`_prefix`](https://github.com/pre-commit/pre-commit/blob/dbd69af90daba11e719e501c4d17ed710c13110d/pre_commit/repository.py#L149-L152)), because when running the hook, we must still know where the virtual environment is and that information is only transported via `hook.prefix` I think. So the only way I see to amend the path is via `hook.src`.

*** 
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):

* local repos work for script paths (prefix is cloned repo).
* remote repos **don't work for script paths** (prefix is cloned repo). -> Hook authors can expose CLI, problem circumvented.

script hooks (prefix is prepended to path in run_hook):

* local repos work for script paths (prefix is cwd).
* remote repos work for script paths (prefix is cloned repo).

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 `language: script` and the other languages like python -.-.

***

<a name="myfootnote1"><sup>1</sup></a>:  I am assuming this new language `rscript` would only allows paths to scripts, no R expressions, for local and remote hooks. 

asottile: ah ok, when I mentioned the thing about `PATH` I meant more that r things don't really have a concept of a thing that installs a CLI tool (`venv/bin` doesn't have an equivalent `renv/bin`)

in that case, it's probably ok to make `R` a little special here and do similar to what this PR is doing 👍 (feel free to use `.src` even though it wasn't intended for this)
lorenzwalthert: Thanks Anthony. Then let's progress with `hook.src`. I am also sorry this whole R support topic needs so much attention from your side and I appreciate every minute you put in to make things happen. R is not a very conventional language and that is exemplified here (neither am I a very experienced Python coder unfortunately).
asottile: happy to spend time on this -- I think R is a valuable addition to the ecosystem (despite it being weird!)
asottile: oops, this conflicts with the other changes
lorenzwalthert: ... which I caused 🙃. Fixed. 

Patch:
diff --git a/pre_commit/languages/r.py b/pre_commit/languages/r.py
index bae4001..d573775 100644
--- a/pre_commit/languages/r.py
+++ b/pre_commit/languages/r.py
@@ -40,14 +40,19 @@ def _get_env_dir(prefix: Prefix, version: str) -> str:
     return prefix.path(helpers.environment_dir(ENVIRONMENT_DIR, version))
 
 
-def _prefix_if_file_entry(
+def _prefix_if_non_local_file_entry(
     entry: Sequence[str],
     prefix: Prefix,
+    src: str,
 ) -> Sequence[str]:
     if entry[1] == '-e':
         return entry[1:]
     else:
-        return (prefix.path(entry[1]),)
+        if src == 'local':
+            path = entry[1]
+        else:
+            path = prefix.path(entry[1])
+        return (path,)
 
 
 def _entry_validate(entry: Sequence[str]) -> None:
@@ -75,7 +80,7 @@ def _cmd_from_hook(hook: Hook) -> Tuple[str, ...]:
 
     return (
         *entry[:1], *RSCRIPT_OPTS,
-        *_prefix_if_file_entry(entry, hook.prefix),
+        *_prefix_if_non_local_file_entry(entry, hook.prefix, hook.src),
         *hook.args,
     )
 


Test patch:
diff --git a/tests/languages/r_test.py b/tests/languages/r_test.py
index 5c046ef..66aa7b3 100644
--- a/tests/languages/r_test.py
+++ b/tests/languages/r_test.py
@@ -14,10 +14,12 @@ def _test_r_parsing(
     hook_id,
     expected_hook_expr={},
     expected_args={},
+    config={},
+    expect_path_prefix=True,
 ):
     repo_path = 'r_hooks_repo'
     path = make_repo(tempdir_factory, repo_path)
-    config = make_config_from_repo(path)
+    config = config or make_config_from_repo(path)
     hook = _get_hook_no_install(config, store, hook_id)
     ret = r._cmd_from_hook(hook)
     expected_cmd = 'Rscript'
@@ -25,7 +27,8 @@ def _test_r_parsing(
         '--no-save', '--no-restore', '--no-site-file', '--no-environ',
     )
     expected_path = os.path.join(
-        hook.prefix.prefix_dir, '.'.join([hook_id, 'R']),
+        hook.prefix.prefix_dir if expect_path_prefix else '',
+        f'{hook_id}.R',
     )
     expected = (
         expected_cmd,
@@ -102,3 +105,25 @@ def test_r_parsing_expr_non_Rscirpt(tempdir_factory, store):
 
     msg = execinfo.value.args
     assert msg == ('entry must start with `Rscript`.',)
+
+
+def test_r_parsing_file_local(tempdir_factory, store):
+    path = 'path/to/script.R'
+    hook_id = 'local-r'
+    config = {
+        'repo': 'local',
+        'hooks': [{
+            'id': hook_id,
+            'name': 'local-r',
+            'entry': f'Rscript {path}',
+            'language': 'r',
+        }],
+    }
+    _test_r_parsing(
+        tempdir_factory,
+        store,
+        hook_id=hook_id,
+        expected_hook_expr=(path,),
+        config=config,
+        expect_path_prefix=False,
+    )