Skip to content
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.

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

Allow orderly_shared_resource to be used with unnamed arguments. #136

Merged
merged 3 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
51 changes: 24 additions & 27 deletions R/metadata.R
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ static_orderly_dependency <- function(args) {
has_name <- !is.null(static_name) || is.null(name)

name <- static_string(name)
files <- static_character_vector(files, TRUE)
files <- fill_missing_names(static_character_vector(files, TRUE))

if (is.character(args$query)) {
query <- static_string(query)
Expand All @@ -325,9 +325,16 @@ static_orderly_dependency <- function(args) {
##'
##' @title Copy shared resources into a packet directory
##'
##' @param ... Named arguments corresponding to shared resources to
##' copy. The name will be the destination filename, while the value
##' is the filename within the shared resource directory.
##' @param ... The shared resources to copy. If arguments are named, the name
##' will be the destination file while the value is the filename within the
##' shared resource directory.
##'
##' You can use a limited form of string interpolation in the names of
##' this argument; using `${variable}` will pick up values from
##' `envir` and substitute them into your string. This is similar
##' to the interpolation you might be familiar with from
##' `glue::glue` or similar, but much simpler with no concatenation
##' or other fancy features supported.
##'
##' @return Invisibly, a data.frame with columns `here` (the fileames
##' as as copied into the running packet) and `there` (the filenames
Expand All @@ -336,7 +343,16 @@ static_orderly_dependency <- function(args) {
##'
##' @export
orderly_shared_resource <- function(...) {
files <- validate_shared_resource(list(...), environment())
files <- validate_file_from_to(
list(...), parent.frame(),
name = "arguments to 'orderly_shared_resource'",
call = environment())

if (nrow(files) == 0) {
cli::cli_abort("'orderly_shared_resource' requires at least one argument",
call = environment())
}

ctx <- orderly_context(rlang::caller_env())

files <- copy_shared_resource(ctx$root_src, ctx$path, ctx$config, files,
Expand All @@ -351,25 +367,6 @@ orderly_shared_resource <- function(...) {
invisible(files)
}


validate_shared_resource <- function(args, call) {
if (length(args) == 0) {
cli::cli_abort("'orderly_shared_resource' requires at least one argument",
call = call)
}
assert_named(args, unique = TRUE, call = environment())
is_invalid <- !vlapply(args, function(x) is.character(x) && length(x) == 1)
if (any(is_invalid)) {
cli::cli_abort(
sprintf(
"Invalid shared resource %s: entries must be strings",
paste(squote(names(args)[is_invalid]), collapse = ", ")),
call = call)
}
list_to_character(args)
}


copy_shared_resource <- function(path_root, path_dest, config, files, call) {
shared_dir <- "shared"
shared_path <- file.path(path_root, shared_dir)
Expand All @@ -379,8 +376,8 @@ copy_shared_resource <- function(path_root, path_dest, config, files, call) {
shared_dir))
}

here <- names(files)
there <- unname(files)
here <- files$here
there <- files$there

assert_file_exists_relative(there, workdir = shared_path,
name = "Shared resource file", call = call)
Expand All @@ -405,7 +402,7 @@ copy_shared_resource <- function(path_root, path_dest, config, files, call) {


static_orderly_shared_resource <- function(args) {
unlist(lapply(args, static_character_vector, TRUE), FALSE, TRUE)
fill_missing_names(unlist(lapply(args, static_string), FALSE, TRUE))
}


Expand Down
10 changes: 3 additions & 7 deletions R/outpack_misc.R
Original file line number Diff line number Diff line change
Expand Up @@ -85,20 +85,16 @@ validate_file_from_to <- function(x, envir,
x <- list_to_character(x)
}

if (is.character(x)) {
to <- names(x) %||% x
from <- unname(x)
if (any(i <- !nzchar(to))) {
to[i] <- from[i]
}
} else {
if (!is.character(x)) {
cli::cli_abort(
c(sprintf("Unexpected object type for '%s'", name),
x = sprintf("Given object of class %s", collapseq(class(x))),
i = "Expected a (named) character vector"),
call = call)
}

to <- names(fill_missing_names(x))
from <- unname(x)
to_value <- string_interpolate_simple(to, envir, call)

if (any(duplicated(to_value))) {
Expand Down
10 changes: 10 additions & 0 deletions R/util.R
Original file line number Diff line number Diff line change
Expand Up @@ -644,3 +644,13 @@ is_testing <- function() {
# https://github.com/r-lib/testthat/blob/fe50a22/R/test-env.R#L20
identical(Sys.getenv("TESTTHAT"), "true")
}

#' Given a character vector, missing names are filled using the value.
fill_missing_names <- function(x) {
if (is.null(names(x))) {
names(x) <- x
} else if (any(i <- !nzchar(names(x)))) {
names(x)[i] <- x[i]
}
x
}
19 changes: 13 additions & 6 deletions R/util_assert.R
Original file line number Diff line number Diff line change
Expand Up @@ -44,18 +44,25 @@ assert_simple_scalar_atomic <- function(x, name = deparse(substitute(x)),
invisible(x)
}

assert_unique_names <- function(x, name = deparse(substitute(x)),
arg = name, call = NULL) {
if (any(duplicated(names(x)))) {
dups <- unique(names(x)[duplicated(names(x))])
cli::cli_abort(
c("'{name}' must have unique names",
i = "Found {length(dups)} duplicate{?s}: {collapseq(dups)}"),
call = call, arg = arg)
}
}

assert_named <- function(x, unique = FALSE, name = deparse(substitute(x)),
arg = name, call = NULL) {
## TODO: we get bad quotes here from static_orderly_parameters
if (is.null(names(x))) {
cli::cli_abort("'{name}' must be named", call = call, arg = arg)
}
if (unique && any(duplicated(names(x)))) {
dups <- unique(names(x)[duplicated(names(x))])
cli::cli_abort(
c("'{name}' must have unique names",
i = "Found {length(dups)} duplicate{?s}: {collapseq(dups)}"),
call = call, arg = arg)
if (unique) {
assert_unique_names(x, name = name, arg = arg, call = call)
}
}

Expand Down
7 changes: 7 additions & 0 deletions tests/testthat/examples/shared-shorthand/shared-shorthand.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
orderly2::orderly_shared_resource("data.csv")
orderly2::orderly_artefact("A graph of things", "mygraph.png")

data <- read.csv("data.csv", stringsAsFactors = FALSE)
png("mygraph.png")
plot(data)
dev.off()
4 changes: 2 additions & 2 deletions tests/testthat/helper-orderly.R
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ copy_examples <- function(examples, path_src) {
}

fs::dir_create(path_src)
if (any(c("shared", "shared-dir") %in% examples)) {
if (any(c("shared", "shared-shorthand", "shared-dir") %in% examples)) {
fs::dir_create(file.path(path_src, "shared"))
if ("shared" %in% examples) {
if (any(c("shared", "shared-shorthand") %in% examples)) {
fs::file_copy(test_path("examples/explicit/data.csv"),
file.path(path_src, "shared"))
}
Expand Down
23 changes: 23 additions & 0 deletions tests/testthat/test-cleanup.R
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,29 @@ test_that("can clean up shared resources", {
expect_equal(status$delete, "shared_data.csv")
})

test_that("can clean up shared resources with shorthand syntax", {
path <- test_prepare_orderly_example("shared-shorthand")
path_src <- file.path(path, "src", "shared-shorthand")
file.create(file.path(path_src, "data.csv"))
status <- orderly_cleanup_status("shared-shorthand", root = path)

files <- c("data.csv", "shared-shorthand.R")
expect_setequal(rownames(status$role), files)
expect_equal(
status$role,
cbind(orderly = set_names(c(FALSE, TRUE), files),
resource = FALSE,
shared_resource = c(TRUE, FALSE),
dependency = FALSE,
artefact = FALSE))
expect_equal(
status$status,
cbind(source = set_names(c(FALSE, TRUE), files),
derived = c(TRUE, FALSE),
ignored = NA))
expect_equal(status$delete, "data.csv")
})


test_that("can clean up dependencies", {
path <- test_prepare_orderly_example(c("data", "depends"))
Expand Down
16 changes: 16 additions & 0 deletions tests/testthat/test-read.R
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@ test_that("read dependency", {
args <- list(name = NULL, query = "latest", files = c(x = "y"))
expect_equal(static_orderly_dependency(args), args)

args <- list(name = NULL, query = "latest", files = c("x"))
expect_equal(static_orderly_dependency(args)$files, c(x = "x"))

args <- list(name = NULL, query = "latest", files = c("x", y = "z"))
expect_equal(static_orderly_dependency(args)$files, c(x = "x", y = "z"))

expect_null(
static_orderly_dependency(list(name = quote(a),
query = "latest",
Expand Down Expand Up @@ -123,3 +129,13 @@ test_that("can parse expressions that might be interesting", {
list(is_orderly = FALSE,
expr = quote(f(a, b, c))))
})

test_that("read shared resource", {
expect_equal(static_orderly_shared_resource(list("a", "b")),
c(a = "a", b = "b"))

expect_equal(static_orderly_shared_resource(list(a = "x", "b")),
c(a = "x", b = "b"))

expect_null(static_orderly_shared_resource(list(quote(c("a", "b")))))
})
78 changes: 70 additions & 8 deletions tests/testthat/test-run.R
Original file line number Diff line number Diff line change
Expand Up @@ -261,19 +261,56 @@ test_that("can run manually with shared resources", {
})


test_that("can run with shared resources using shorthand arguments", {
path <- test_prepare_orderly_example("shared-shorthand")
envir <- new.env()
id <- orderly_run_quietly("shared-shorthand", root = path, envir = envir)
expect_setequal(
dir(file.path(path, "archive", "shared-shorthand", id)),
c("data.csv", "mygraph.png", "shared-shorthand.R"))
meta <- orderly_metadata(id, root = path)
expect_equal(nrow(meta$custom$orderly$shared), 1)
expect_equal(meta$custom$orderly$shared,
data_frame(here = "data.csv", there = "data.csv"))
expect_equal(
meta$custom$orderly$role,
data_frame(path = c("shared-shorthand.R", "data.csv"),
role = c("orderly", "shared")))
})

test_that("can validate shared resource arguments", {
expect_error(
validate_shared_resource(list(), NULL),
orderly_shared_resource(),
"'orderly_shared_resource' requires at least one argument")

expect_error(
validate_shared_resource(list(input = c("a", "b")), NULL),
"Invalid shared resource 'input': entries must be strings")
orderly_shared_resource(c("a", "b")),
"All elements of 'arguments to 'orderly_shared_resource'' must be strings")
expect_error(
validate_shared_resource(list(a = 1, b = TRUE, c = "str"), NULL),
"Invalid shared resource 'a', 'b': entries must be strings")
expect_equal(
validate_shared_resource(list(a = "A", b = "B"), NULL),
c(a = "A", b = "B"))
orderly_shared_resource(a = 1, b = TRUE, c = "str"),
"All elements of 'arguments to 'orderly_shared_resource'' must be strings")
expect_error(
orderly_shared_resource(1, TRUE, "str"),
"All elements of 'arguments to 'orderly_shared_resource'' must be strings")
expect_error(
orderly_shared_resource(a = 1, TRUE, "str"),
"All elements of 'arguments to 'orderly_shared_resource'' must be strings")
Copy link
Member Author

@plietar plietar Apr 16, 2024

Choose a reason for hiding this comment

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

These messages are pretty terrible, but I couldn't find a way to deparse the ... reliably so I've just used a placeholder instead.

I'd almost be tempted to change orderly_shared_resource to accept a single files argument instead, making it consistent with all the other functions, including orderly_dependency, orderly_copy_files and orderly_resource, and would make the deparsing easy, but of course that's a breaking change.


expect_error(
orderly_shared_resource(a = "A", a = "B"),
paste("Every destination filename (in 'arguments to",
"'orderly_shared_resource'') must be unique"),
fixed = TRUE)
expect_error(
orderly_shared_resource("a", "a"),
paste("Every destination filename (in 'arguments to",
"'orderly_shared_resource'') must be unique"),
fixed = TRUE)
expect_error(
orderly_shared_resource("a", a = "B"),
paste("Every destination filename (in 'arguments to",
"'orderly_shared_resource'') must be unique"),
fixed = TRUE)
})


Expand Down Expand Up @@ -1243,6 +1280,31 @@ test_that("can read about assigned shared resources", {
expect_equal(res$shared_resource, c(shared_data = "data"))
})

test_that("can read about shared resources with shorthand syntax", {
path <- test_prepare_orderly_example("shared-shorthand")
path_src <- file.path(path, "src", "shared-shorthand")
code <- readLines(file.path(path_src, "shared-shorthand.R"))
code <- sub("orderly2::orderly_shared_resource",
"r <- orderly2::orderly_shared_resource",
code)
code <- c(code, 'saveRDS(r, "resources.rds")')
writeLines(code, file.path(path_src, "shared-shorthand.R"))

id <- orderly_run_quietly("shared-shorthand", root = path, envir = new.env())
r <- readRDS(file.path(path, "archive", "shared-shorthand",
id, "resources.rds"))
expect_equal(r, data_frame(here = "data.csv", there = "data.csv"))

res <- withr::with_dir(
path_src,
withVisible(orderly_shared_resource("data.csv")))
expect_equal(res$visible, FALSE)
expect_equal(res$value, r)

res <- orderly_read(path_src)
expect_equal(res$shared_resource, c(data.csv = "data.csv"))
})


test_that("can read about dependencies", {
path <- test_prepare_orderly_example(c("data", "depends"))
Expand Down
8 changes: 8 additions & 0 deletions tests/testthat/test-util-assert.R
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,14 @@ test_that("assert_simple_scalar_atomic", {
expect_error(assert_simple_scalar_atomic(list(1)), "must be atomic")
})

test_that("assert_unique_names", {
expect_error(assert_unique_names(setNames(1:3, c("a", "a", "c"))),
"must have unique names")
expect_error(assert_unique_names(setNames(1:3, c("a", "c", "a"))),
"must have unique names")
expect_silent(assert_unique_names(setNames(1:3, c("a", "b", "c"))))
})


test_that("assert_named", {
expect_error(assert_named(1), "must be named")
Expand Down
7 changes: 7 additions & 0 deletions tests/testthat/test-util.R
Original file line number Diff line number Diff line change
Expand Up @@ -334,3 +334,10 @@ test_that("read_string strips newlines", {
result <- expect_silent(read_string(path))
expect_equal(result, "12345678")
})

test_that("fill_missing_names works", {
expect_equal(fill_missing_names(NULL), NULL)
expect_equal(fill_missing_names(c("a", "b")), c(a = "a", b = "b"))
expect_equal(fill_missing_names(c("a", "a")), c(a = "a", a = "a"))
expect_equal(fill_missing_names(c(x = "a", "a")), c(x = "a", a = "a"))
})
2 changes: 1 addition & 1 deletion vignettes/introduction.Rmd
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ We can then write an orderly report `use_shared` that uses this shared file, wit
```{r, echo = FALSE, results = "asis"}
fs::dir_create(file.path(path, "src", "use_shared"))
writeLines(c(
'orderly2::orderly_shared_resource(data.csv = "data.csv")',
'orderly2::orderly_shared_resource("data.csv")',
'orderly2::orderly_artefact("analysis", "analysis.png")',
"",
'd <- read.csv("data.csv")',
Expand Down
Loading