Support working-directory options for hooks

[TNonet]

Is your feature request related to a problem? Please describe.

I would like a way to run many of these pre-commit hooks in arbitrary subdirectories.

I am creating a repository with the following layout:

.
├── common/
│   └── ...
├── pypkg/
│   ├── setup.py
│   └── ...
├── rpkg/
│   ├── DESCRIPTION
│   └── ...
└── ...

Currently, use-tidy-description, deps-in-desc, and a few other hooks will fail with this repo layout.

Describe the solution you'd like
Each pre-commit hook (if possible) has either a before script that can allow users to change directories or support some type of argument for specifying the working-directory similar to github actions.

Describe alternatives you've considered
An alternative approach would be to support .pre-commit-config.yaml files in subdirectories but that seems out of scope for this project.

[lorenzwalthert]

Thanks @TNonet. Indeed, this repo layout does not work with the hook {precommit} provides. Pre-commit uses command line arguments to specify hook behaviour. Hence that's the natural solution I think. We could just pass a root argument to these hooks. Are you interested in making a PR?

[TNonet]

Hi @lorenzwalthert,

Thanks for such a fast response. I would be happy to make a PR.

After a quick scan of the logic, I would plan to add a --root option to each hook as a docopt option. Then use R's setwd at the top of each script. Does this seem reasonable?

A few other comments/questions:

1. Some hooks don't have docopt arguments; is there any nuance to initially adding to a hook's script?
2. The testing for this package is somewhat abstracted. Any suggestions on writing tests to ensure this new argument doesn't cause unintended side effects? I would rather not mock-up more files in the testing/in directory.

[lorenzwalthert]

Does this seem reasonable?

Because the file names are passed to the hook as relative paths, you may need to convert them to absolute paths (with base R preferably) before setting the working directory.

Some hooks don't have docopt arguments; is there any nuance to initially adding to a hook's script?

I don’t think so.

regarding tests, that’s a bit of a tricky one. Testing infra has grown organically to cover all use cases and os a bit of a mile field 😜. I think I can set it up for the root argument next week.

Also, I wonder if we should add the argument to all hooks, no matter if it has an effect or not. Just for completeness.

[lorenzwalthert]

@TNonet you can see #432 for how we could adapt hooks and tests. Do you want to continue in that fashion? I think we should only add the --root argument to hooks where it matters, i.e. not for styler etc.

[TNonet]

Hi @lorenzwalthert,

Thanks for the example. I will see what I can do.

[TNonet]

It seems that adding docopt to use-tidy-description is causing some issues with the testing suite.

I added the following lines to inst/hooks/exported/use-tidy-descriptions.R. Note the lack of arguments that should not influence the execution of the hook. It also fails with a simple --root argument similar to #432.

"Usage:
  use-tidy-description
" -> doc

arguments <- docopt::docopt(doc)

And I received the following unexpected failure.

> testthat::test_file(
+     "tests/testthat/test-hooks.R",
+     reporter = testthat::MultiReporter$new(list(
+         testthat::CheckReporter$new(), testthat::FailReporter$new()
+     ))
+ )
[ FAIL 1 | WARN 0 | SKIP 0 | PASS 1 ]

══ Failed tests ═══════════════════════════════════════════════════════════════════════════════════════════════
── Failure (test-hooks.R:5:1): (code run outside of `test_that()`) ─────────────
Expected: No error. Found:
Error: Usage:
  use-tidy-description
Execution halted
Backtrace:
    ▆
 1. └─precommit::run_test("use-tidy-description", "DESCRIPTION", suffix = "") at test-hooks.R:5:0
 2.   └─precommit::run_test_impl(...) at precommit/R/testing.R:57:2
 3.     └─precommit::hook_state_assert(...) at precommit/R/testing.R:118:2
 4.       └─purrr::map2(...) at precommit/R/testing.R:168:2
 5.         └─precommit .f(.x[[1L]], .y[[1L]], ...)

[ FAIL 1 | WARN 0 | SKIP 0 | PASS 1 ]
Error: Failures detected.

I have yet to make my way through the internal logic of the testing, but I wanted to raise this issue just in case it jumps out at you.

[lorenzwalthert]

Ok, it was related to an error message that now has an absolute path. Fixed and re-based on main for a cleaner diff... All passing.

[TNonet]

Hi @lorenzwalthert,

Sorry for the delay on this. I had covid for a few weeks.

I still see this error below when running tests on both #432 and main. However, the code seems to run correctly when executing the R hook script on packages:

It seems that adding docopt to use-tidy-description is causing some issues with the testing suite.

I added the following lines to inst/hooks/exported/use-tidy-descriptions.R. Note the lack of arguments that should not influence the execution of the hook. It also fails with a simple --root argument similar to #432.

"Usage:
  use-tidy-description
" -> doc

arguments <- docopt::docopt(doc)

And I received the following unexpected failure.

> testthat::test_file(
+     "tests/testthat/test-hooks.R",
+     reporter = testthat::MultiReporter$new(list(
+         testthat::CheckReporter$new(), testthat::FailReporter$new()
+     ))
+ )
[ FAIL 1 | WARN 0 | SKIP 0 | PASS 1 ]

══ Failed tests ═══════════════════════════════════════════════════════════════════════════════════════════════
── Failure (test-hooks.R:5:1): (code run outside of `test_that()`) ─────────────
Expected: No error. Found:
Error: Usage:
  use-tidy-description
Execution halted
Backtrace:
    ▆
 1. └─precommit::run_test("use-tidy-description", "DESCRIPTION", suffix = "") at test-hooks.R:5:0
 2.   └─precommit::run_test_impl(...) at precommit/R/testing.R:57:2
 3.     └─precommit::hook_state_assert(...) at precommit/R/testing.R:118:2
 4.       └─purrr::map2(...) at precommit/R/testing.R:168:2
 5.         └─precommit .f(.x[[1L]], .y[[1L]], ...)

[ FAIL 1 | WARN 0 | SKIP 0 | PASS 1 ]
Error: Failures detected.

I have yet to make my way through the internal logic of the testing, but I wanted to raise this issue just in case it jumps out at you.

[lorenzwalthert]

oh no, sorry to hear that. Hope everything is ok now…

All CI passes, so I don’t know what’s the problem is on your side. Maybe update all your R packages?