Skip to content

Allow a root for deps-in-desc #432

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Aug 2, 2022
Merged

Allow a root for deps-in-desc #432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4ba4a29
support argument root for deps in desc
lorenzwalthert Jul 10, 2022
47b6df4
fix visual
lorenzwalthert Jul 11, 2022
82d79fb
fix spacing
lorenzwalthert Jul 11, 2022
a519609
file paths are now absolute in error messages too and `std_out` check…
lorenzwalthert Jul 11, 2022
1a42691
ensure cannonical temp dir
lorenzwalthert Aug 1, 2022
60da741
ensure that source files are in root/rpkg, i.e. where the R package i…
lorenzwalthert Aug 1, 2022
83914bd
avoid devtools dep
lorenzwalthert Aug 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/hook-tests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ jobs:
}
# avoid build-time deps
options(install.packages.compile.from.source = "never")
renv::install(c('testthat', 'devtools', 'git2r'))
renv::install(c('testthat', 'pkgload', 'git2r'))
# some testing infrastructure we need is in R/testing.R
renv::install(getwd())
shell: Rscript {0}
Expand All @@ -101,7 +101,7 @@ jobs:
shell: Rscript {0}
- name: Test
run: |
devtools::load_all()
pkgload::load_all()
testthat::test_file(
"tests/testthat/test-hooks.R",
reporter = testthat::MultiReporter$new(list(
Expand Down
3 changes: 2 additions & 1 deletion R/testing.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ run_test_impl <- function(path_executable,
file_transformer,
env,
expect_success) {
tempdir <- fs::dir_create(fs::file_temp())
# ensure cannonical /private/var/... not /var/... on macOS
tempdir <- fs::path(normalizePath((fs::dir_create(fs::file_temp()))))
copy_artifacts(artifacts, tempdir)
# if name set use this, otherwise put in root
path_candidate_temp <- fs::path(tempdir, names(path_candidate))
Expand Down
7 changes: 6 additions & 1 deletion inst/hooks/exported/deps-in-desc.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@

"Ensure all dependencies of the form pkg::fun are in DESCRIPTION
Usage:
deps-in-desc [--allow_private_imports] <files>...
deps-in-desc [--allow_private_imports] [--root=<root_>] <files>...

Options:
--allow_private_imports Whether or not to allow the use of ::: on imported functions.
--root=<root_> Path relative to the git root that contains the R package root [default: .].

" -> doc
pre_installed <- c(
"base", "boot", "class", "cluster", "codetools", "compiler",
Expand All @@ -16,6 +18,8 @@ pre_installed <- c(
)

arguments <- docopt::docopt(doc)
arguments$files <- normalizePath(arguments$files) # because working directory changes to root
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we want to normalize these paths. Doesn't that remove the purpose of this --root option?

If we normalize files before setwd(normalizePath(arguments$root)), then this hook called like below:

deps-in-desc --root "rpkg" FILE1

will process rpkg/../FILE1

However, I think we would want it to process rpkg/FILE1

Therefore, this would require:

deps-in-desc --root "rpkg" rpkg/FILE1

Am I missing something?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I can follow, but pre-commit will generate a call where the paths to the files are relative to the git root. Like this

Rscript inst/hooks/exported/deps-in-desc.R --root rpkg rpkg/file1

So we need to make these paths absolute before changing the working directory, otherwise information is lost. Afterwards, it does not matter anymore if the paths are absoulte or if we convert back to relative. The working directory change is only for ensuring desc::desc() finds the DESCRIPTION file. And it could also be solved without changing the working directory in the first place but I think the current approach genearlised better to other situations.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed a fix for tests because previously, the R source file to test was not in the R sub directory, but one level above. This also works, i.e. the R source file can be outside of the R package root to test if the dependencies are captured, but probably that's not what we want to test.

Copy link
Contributor

@TNonet TNonet Aug 2, 2022 •
edited by lorenzwalthert
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I can follow, but pre-commit will generate a call where the paths to the files are relative to the git root. Like this

Rscript inst/hooks/exported/deps-in-desc.R --root rpkg rpkg/file1

So we need to make these paths absolute before changing the working directory, otherwise information is lost. Afterwards, it does not matter anymore if the paths are absoulte or if we convert back to relative. The working directory change is only for ensuring desc::desc() finds the DESCRIPTION file. And it could also be solved without changing the working directory in the first place but I think the current approach genearlised better to other situations.

Ah, I thought the files were being passed in by the user and not from precommit itself. I now understand the need to normalize the files.

setwd(normalizePath(arguments$root))
deps_in_desc <- function(file, arguments) {
if (basename(file) == "README.Rmd") {
# is .Rbuildignored, dependencies irrelevant
Expand Down Expand Up @@ -70,6 +74,7 @@ deps_in_desc <- function(file, arguments) {
}
out
}

out <- lapply(arguments$files, deps_in_desc, arguments = arguments)
if (!all(unlist(out))) {
stop("Dependency check failed", call. = FALSE)
Expand Down
31 changes: 30 additions & 1 deletion tests/testthat/test-hooks.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,35 @@ run_test("deps-in-desc",
artifacts = c("DESCRIPTION" = test_path("in/DESCRIPTION"))
)

# in sub directory with wrong root
run_test("deps-in-desc",
suffix = "-fail.R", std_err = "contains a file",
file_transformer = function(files) {
fs::path_abs(fs::file_move(files, "rpkg"))
},
artifacts = c("rpkg/DESCRIPTION" = test_path("in/DESCRIPTION"))
)

# in sub directory with correct root
run_test("deps-in-desc",
cmd_args = "--root=rpkg",
suffix = "-fail.R", std_err = "Dependency check failed",
file_transformer = function(files) {
fs::path_abs(fs::file_move(files, "rpkg"))
},
artifacts = c("rpkg/DESCRIPTION" = test_path("in/DESCRIPTION"))
)

# in sub directory with correct root
run_test("deps-in-desc",
cmd_args = "--root=rpkg",
suffix = "-success.R", std_err = NULL,
file_transformer = function(files) {
fs::path_abs(fs::file_move(files, "rpkg"))
},
artifacts = c("rpkg/DESCRIPTION" = test_path("in/DESCRIPTION"))
)

# with :::
run_test("deps-in-desc",
"deps-in-desc-dot3",
Expand All @@ -237,7 +266,7 @@ run_test("deps-in-desc",
run_test("deps-in-desc",
"deps-in-desc",
suffix = "-fail.Rmd", std_err = "Dependency check failed",
std_out = "in file `deps-in-desc-fail.Rmd`",
std_out = "deps-in-desc-fail.Rmd`: ttyzp",
artifacts = c("DESCRIPTION" = test_path("in/DESCRIPTION"))
)

Expand Down
24 changes: 20 additions & 4 deletions vignettes/available-hooks.Rmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,14 +226,30 @@ roclets you specified in `DESCRIPTION`.
## `deps-in-desc`

Checks if packages used with the `pkgname::fun()` syntax are listed in
your DESCRIPTION file. Note that `README.Rmd` is never checked. Flag
`allow_private_imports` lets the user specify that private imports into
the package namespace are tolerable, e.g. `somepkg:::x()`. Flag not set
by default, i.e. the hook will fail if such a call is found.
your DESCRIPTION file. Note that `README.Rmd` is never checked.

**Arguments**

- Flag `allow_private_imports` lets the user specify that private
imports into the package namespace are tolerable, e.g.
`somepkg:::x()`. Flag not set by default, i.e. the hook will fail if
such a call is found.

<!-- -->

id: deps-in-desc
args: [--allow_private_imports]

- Argument `root` specifies the directory in the git repo that
contains the R package. Defaults to `.` since for most R package git
repos, the git and R package root coincide. Added in version
0.3.2.9000.

<!-- -->

id: deps-in-desc
args: [--root=<R package root>]

This hook does not modify the file `DESCRIPTION` because the user should
decide for each package if it should go to `Imports:` or `Suggests:`,
which can be done easily with `usethis::use_package()`.
Expand Down