Ergonomic improvements for querying, part 2

DESCRIPTION

@@ -1,6 +1,6 @@
 Package: orderly2
 Title: Orderly Next Generation
-Version: 1.99.45
+Version: 1.99.46
 Authors@R: c(person("Rich", "FitzJohn", role = c("aut", "cre"),
                     email = "[email protected]"),
              person("Robert", "Ashton", role = "aut"),

R/interactive.R

@@ -75,13 +75,16 @@ detect_orderly_interactive_path <- function(
 ##'
 ##' @title Set search options for interactive use
 ##'
-##' @param options Optional control over locations, when used
-##'   with [orderly2::orderly_dependency]; see of Details section of
-##'   [orderly2::orderly_run].
+##' @inheritParams orderly_search_options
 ##'
 ##' @return Nothing, called for its side effects
 ##' @export
-orderly_interactive_set_search_options <- function(options = NULL) {
+orderly_interactive_set_search_options <- function(location = NULL,
+                                                   allow_remote = NULL,
+                                                   pull_metadata = FALSE) {
+  options <- build_search_options(location = location,
+                                  allow_remote = allow_remote,
+                                  pull_metadata = pull_metadata)
   .interactive$search_options <- options
 }
 

R/location.R

@@ -323,49 +323,44 @@ orderly_location_pull_metadata <- function(location = NULL, root = NULL) {
 ##'
 ##' @title Pull one or more packets from a location
 ##'
-##' @param ... Arguments passed through to
-##'   [orderly2::orderly_search]. In the special case where the first
-##'   argument is a character vector of ids *and* there are no named
-##'   dot arguments, then we interpret this argument as a vector of
-##'   ids directly. Be careful here, your query may pull a lot of data
-##'   - in particular, passing `NULL` will match everything that every
-##'   remote has!
-##'
-##' @param options Options passed to [orderly2::orderly_search].
-##'   The option `allow_remote` must be `TRUE` as otherwise no packet could
-##'   possibly be pulled, so an error is thrown if this is FALSE.
-##'
 ##' @param recursive If non-NULL, a logical, indicating if we should
 ##'   recursively pull all packets that are referenced by the packets
 ##'   specified in `id`.  This might copy a lot of data!  If `NULL`,
 ##'   we default to the value given by the the configuration option
 ##'   `require_complete_tree`.
 ##'
 ##' @inheritParams orderly_metadata
+##' @inheritParams orderly_search
+##' @inheritParams orderly_search_options
 ##'
 ##' @return Invisibly, the ids of packets that were pulled
 ##' @export
-orderly_location_pull_packet <- function(..., options = NULL, recursive = NULL,
+orderly_location_pull_packet <- function(expr,
+                                         name = NULL,
+                                         location = NULL,
+                                         pull_metadata = FALSE,
+                                         recursive = NULL,
+                                         options = NULL,
                                          root = NULL) {
   root <- root_open(root, require_orderly = FALSE)
-  options <- as_orderly_search_options(options, list(allow_remote = TRUE))
-  if (!options$allow_remote) {
-    cli::cli_abort(
-      "If specifying 'options', 'allow_remote' must be TRUE",
-      i = "If FALSE, then we can't find a packet you don't already have :)")
-  }
-  if (dots_is_literal_id(...)) {
-    ids <- ..1
+  compatibility_fix_options(options, "orderly_location_pull_packet")
+
+  if (expr_is_literal_id(expr, name)) {
+    ids <- expr
   } else {
-    ids <- orderly_search(..., options = options, root = root)
+    ids <- orderly_search(expr,
+                          name = name,
+                          location = location,
+                          allow_remote = TRUE,
+                          pull_metadata = pull_metadata,
+                          root = root)
   }
 
-  if (length(ids) == 0) {
-    if (options$allow_remote && !options$pull_metadata) {
-      pull_arg <- gsub(" ", "\u00a0", "options = list(pull_metadata = TRUE)")
+  if (length(ids) == 0 || (length(ids) == 1 && is.na(ids))) {
+    if (!pull_metadata) {
       hint <- c(i = paste("Did you forget to pull metadata? You can do this",
-                          "by using the argument '{pull_arg}' in the call",
-                          "to 'orderly_location_pull_packet()', or",
+                          "by using the argument {.code pull_metadata = TRUE}",
+                          "in the call to 'orderly_location_pull_packet()', or",
                           "by running 'orderly_location_pull_metadata()'"))
     } else {
       hint <- NULL
@@ -377,7 +372,7 @@ orderly_location_pull_packet <- function(..., options = NULL, recursive = NULL,
         hint))
   }
 
-  plan <- location_build_pull_plan(ids, options$locations, recursive, root,
+  plan <- location_build_pull_plan(ids, location, recursive, root,
                                    call = environment())
 
   if (plan$info$n_extra > 0) {

R/metadata.R

@@ -285,17 +285,29 @@ orderly_dependency <- function(name, query, files) {
   ctx <- orderly_context(rlang::caller_env())
   subquery <- NULL
   query <- orderly_query(query, name = name, subquery = subquery)
-  search_options <- as_orderly_search_options(ctx$search_options)
+  search_options <- ctx$search_options %||% build_search_options()
+
+  ## TODO: this separation of codepaths here is quite weird.  We
+  ## should do the copy here and have the outpack function probably
+  ## just do the metadata update.  The logic is otherwise fine I
+  ## think.
   if (ctx$is_active) {
     res <- outpack_packet_use_dependency(ctx$packet, query, files,
                                          search_options = search_options,
                                          envir = ctx$envir,
                                          overwrite = TRUE)
   } else {
     res <- orderly_copy_files(
-      query, files = files, dest = ctx$path, overwrite = TRUE,
-      parameters = ctx$parameters, options = search_options,
-      envir = ctx$envir, root = ctx$root)
+      query,
+      files = files,
+      dest = ctx$path,
+      overwrite = TRUE,
+      parameters = ctx$parameters,
+      location = search_options$location,
+      allow_remote = search_options$allow_remote,
+      pull_metadata = search_options$pull_metadata,
+      envir = ctx$envir,
+      root = ctx$root)
   }
 
   cli::cli_alert_info(

R/outpack_helpers.R

@@ -72,43 +72,59 @@
 ##'   typically what you want, but set to `FALSE` if you would prefer
 ##'   that an error be thrown if the destination file already exists.
 ##'
-##' @param ... Additional arguments passed through to [orderly_search]
-##'
 ##' @inheritParams orderly_search
+##' @inheritParams orderly_search_options
 ##' @inheritParams orderly_metadata
 ##'
 ##' @return Nothing, invisibly. Primarily called for its side effect
 ##'   of copying files from a packet into the directory `dest`
 ##'
 ##' @export
-orderly_copy_files <- function(..., files, dest, overwrite = TRUE,
-                               envir = parent.frame(), options = NULL,
+orderly_copy_files <- function(expr, files, dest, overwrite = TRUE,
+                               name = NULL, location = NULL,
+                               allow_remote = NULL, pull_metadata = FALSE,
+                               parameters = NULL, options = NULL,
+                               envir = parent.frame(),
                                root = NULL) {
   root <- root_open(root, require_orderly = FALSE)
+  compatibility_fix_options(options, "orderly_copy_files")
+  ## Validate options here so we can refer to the computed value of
+  ## allow_remote later in error messages.
+  options <- build_search_options(location = location,
+                                  allow_remote = allow_remote,
+                                  pull_metadata = pull_metadata)
 
   ## Validate files and dest early; it gives a better error where this
   ## was not provided with names.
   files <- validate_file_from_to(files, envir)
   assert_scalar_character(dest, call = environment())
 
-  if (dots_is_literal_id(...)) {
-    id <- ..1
+  if (expr_is_literal_id(expr, name)) {
+    id <- expr
     if (length(id) != 1) {
-      cli::cli_abort(sprintf(
-        "Expected a length 1 value for first argument if id (not %d)",
-        length(id)))
+      cli::cli_abort(
+        "Expected a length 1 value for 'expr' if id (not {length(id)})",
+        arg = expr)
     }
   } else {
-    id <- orderly_search(..., options = options, envir = envir, root = root)
+    id <- orderly_search(expr,
+                         name = name,
+                         parameters = parameters,
+                         location = options$location,
+                         allow_remote = options$allow_remote,
+                         pull_metadata = options$pull_metadata,
+                         root = root)
     if (length(id) > 1) {
-      cli::cli_abort(c(
-        sprintf("Query returned %d results, expected a single result",
-                length(id)),
-        i = "Did you forget latest()?"))
+      cli::cli_abort(
+        c("Query returned {length(id)} results, expected a single result",
+          i = "Did you forget latest()?"))
     }
     if (length(id) == 0 || is.na(id)) {
-      explanation <- orderly_query_explain(..., options = options,
-                                           envir = envir, root = root)
+      explanation <- orderly_query_explain(
+        expr, name = name, parameters = parameters,
+        location = options$location,
+        allow_remote = options$allow_remote,
+        envir = envir, root = root)
       cli::cli_abort(
         c("Query returned 0 results",
           i = "See 'rlang::last_error()$explanation' for details"),
@@ -135,11 +151,11 @@ orderly_copy_files <- function(..., files, dest, overwrite = TRUE,
           c("Unable to copy files, due to {reason} packet {id}",
             i = "Consider '{cmd}' to remove this packet from consideration"),
           parent = e)
-      } else if (!as_orderly_search_options(options)$allow_remote) {
+      } else if (!options$allow_remote) {
         cli::cli_abort(
           c("Unable to copy files, as they are not available locally",
             i = paste("To fetch from a location, try again with",
-                      "options = list(allow_remote = TRUE)")),
+                      "{.code allow_remote = TRUE}")),
           parent = e)
       }
       copy_files_from_remote(id, plan$there, plan$here, dest, overwrite, root,

R/outpack_packet.R

@@ -162,7 +162,8 @@ outpack_packet_use_dependency <- function(packet, query, files,
                                           overwrite = TRUE) {
   packet <- check_current_packet(packet)
   query <- as_orderly_query(query, arg = "query")
-  search_options <- as_orderly_search_options(search_options)
+  search_options <- search_options %||% build_search_options()
+  assert_is(search_options, "orderly_search_options")
 
   if (!query$info$single) {
     stop(paste(
@@ -174,12 +175,16 @@ outpack_packet_use_dependency <- function(packet, query, files,
   id <- orderly_search(query,
                        parameters = packet$parameters,
                        envir = envir,
-                       options = search_options,
+                       location = search_options$location,
+                       allow_remote = search_options$allow_remote,
+                       pull_metadata = search_options$pull_metadata,
                        root = packet$root)
   if (is.na(id)) {
     explanation <- orderly_query_explain(
       query, parameters = packet$parameters, envir = envir,
-      options = search_options, root = packet$root)
+      location = search_options$location,
+      allow_remote = search_options$allow_remote,
+      root = packet$root)
     cli::cli_abort(
       c("Failed to find packet for query '{format(query)}'",
         i = "See 'rlang::last_error()$explanation' for details"),
@@ -194,7 +199,9 @@ outpack_packet_use_dependency <- function(packet, query, files,
   }
 
   result <- orderly_copy_files(id, files = files, dest = packet$path,
-                               options = search_options,
+                               location = search_options$location,
+                               allow_remote = search_options$allow_remote,
+                               pull_metadata = search_options$pull_metadata,
                                overwrite = overwrite,
                                envir = envir,
                                root = packet$root)

R/outpack_tools.R

@@ -182,27 +182,33 @@
 ##'
 ##' @title Extract metadata from orderly2 packets
 ##'
-##' @param ... Arguments passed through to
-##'   [orderly2::orderly_search]. In the special case where the first
-##'   argument is a character vector of ids *and* there are no named
-##'   dot arguments, then we interpret this argument as a vector of
-##'   ids directly.
-##'
 ##' @param extract A character vector of columns to extract, possibly
 ##'   named. See Details for the format.
 ##'
 ##' @inheritParams orderly_metadata
+##' @inheritParams orderly_search
+##' @inheritParams orderly_search_options
 ##'
 ##' @return A `data.frame`, the columns of which vary based on the
 ##'   names of `extract`; see Details for more information.
 ##'
 ##' @export
-orderly_metadata_extract <- function(..., extract = NULL, root = NULL) {
+orderly_metadata_extract <- function(expr = NULL, name = NULL, location = NULL,
+                                     allow_remote = NULL, pull_metadata = FALSE,
+                                     extract = NULL, options = NULL,
+                                     root = NULL) {
   root <- root_open(root, require_orderly = FALSE)
-  if (dots_is_literal_id(...)) {
-    ids <- ..1
+  compatibility_fix_options(options, "orderly_metadata_extract")
+
+  if (expr_is_literal_id(expr, name)) {
+    ids <- expr
   } else {
-    ids <- orderly_search(..., root = root)
+    ids <- orderly_search(expr,
+                          name = name,
+                          location = location,
+                          allow_remote = allow_remote,
+                          pull_metadata = pull_metadata,
+                          root = root)
   }
   extract <- parse_extract(extract, environment())
 

R/prune.R

@@ -29,7 +29,7 @@ orderly_prune_orphans <- function(root = NULL) {
     return(invisible(id))
   }
 
-  idx <- new_query_index(root, orderly_search_options(location = local))
+  idx <- new_query_index(root, build_search_options(location = local))
   is_used <- lengths(lapply(id, idx$get_packet_uses, Inf)) > 0
   if (any(is_used)) {
     cli::cli_alert_info(

R/query.R

@@ -49,9 +49,10 @@ orderly_query <- function(expr, name = NULL, scope = NULL, subquery = NULL) {
 }
 
 
-dots_is_literal_id <- function(...) {
-  ...length() == 1 && is.null(...names()) && is.character(..1) &&
-    all(grepl(re_id, ..1))
+expr_is_literal_id <- function(expr, ...) {
+  all(vlapply(list(...), is.null)) &&
+    is.character(expr) &&
+    all(grepl(re_id, expr))
 }
 
 

R/query_explain.R

@@ -6,6 +6,7 @@
 ##' @title Explain a query
 ##'
 ##' @inheritParams orderly_search
+##' @inheritParams orderly_search_options
 ##'
 ##' @return An object of class `orderly_query_explain`, which can be
 ##'   inspected (contents subject to change) and which has a print
@@ -16,12 +17,16 @@
 orderly_query_explain <- function(expr, name = NULL, scope = NULL,
                                   subquery = NULL, parameters = NULL,
                                   envir = parent.frame(),
-                                  options = NULL, root = NULL) {
+                                  location = NULL,
+                                  allow_remote = NULL,
+                                  root = NULL) {
   root <- root_open(root, require_orderly = FALSE)
   query <- as_orderly_query(expr, name, scope, subquery)
-  options <- as_orderly_search_options(options)
   found <- orderly_search(query, parameters = parameters, envir = envir,
-                          options = options, root = root)
+                          location = location,
+                          allow_remote = allow_remote,
+                          pull_metadata = FALSE,
+                          root = root)
   query_simplified <- query_simplify(query)
   ret <- list(found = found,
               n = length(stats::na.omit(found)), # latest() returns NA
@@ -30,8 +35,13 @@ orderly_query_explain <- function(expr, name = NULL, scope = NULL,
 
   for (name in names(query_simplified$parts)) {
     expr <- query_simplified$parts[[name]]
-    found <- orderly_search(expr, parameters = parameters, envir = envir,
-                            options = options, root = root)
+    found <- orderly_search(expr,
+                            parameters = parameters,
+                            envir = envir,
+                            location = location,
+                            allow_remote = allow_remote,
+                            pull_metadata = FALSE,
+                            root = root)
     ret$parts[[name]] <- list(
       name = name,
       str = deparse_query(expr, NULL, NULL),