From 865edca89caafecc4183e4bb4f2c78a98aed2221 Mon Sep 17 00:00:00 2001 From: Rich FitzJohn Date: Fri, 13 Oct 2023 19:26:11 +0100 Subject: [PATCH] Prompt for missing parameters --- DESCRIPTION | 2 +- R/interactive.R | 58 +++++++++++++++++++++++++++++++ R/metadata.R | 16 +++++++++ R/run.R | 15 ++------ man/orderly_parameters.Rd | 18 ++++++++++ tests/testthat/test-interactive.R | 40 +++++++++++++++++++++ tests/testthat/test-parameters.R | 33 +++++++++++++++++- 7 files changed, 167 insertions(+), 15 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index edbc8188..202e48c6 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: orderly2 Title: Orderly Next Generation -Version: 1.99.6 +Version: 1.99.7 Authors@R: c(person("Rich", "FitzJohn", role = c("aut", "cre"), email = "rich.fitzjohn@gmail.com"), person("Robert", "Ashton", role = "aut"), diff --git a/R/interactive.R b/R/interactive.R index d8dedf30..e97200f6 100644 --- a/R/interactive.R +++ b/R/interactive.R @@ -38,3 +38,61 @@ detect_orderly_interactive_path <- function(path = getwd()) { orderly_interactive_set_search_options <- function(options = NULL) { .interactive$search_options <- options } + + +get_parameter_interactive <- function(name, call = NULL) { + value <- readline(sprintf("%s > ", name)) + if (!nzchar(value)) { + cli::cli_abort("Expected a value for parameter '{name}'", call = call) + } + parsed <- tryCatch(parse(text = value)[[1L]], error = identity) + explain_string <- paste( + "If entering a string, you must use quotes, this helps", + 'disambiguate numbers and booleans. For example, TRUE and "TRUE" will', + "parse as a boolean and a string, respectively") + if (inherits(parsed, "error")) { + cli::cli_abort( + c("Failed to parse value for parameter '{name}'", + x = "Was given: {value}", + i = explain_string), + parent = parsed, call = call) + } + if (is.name(parsed)) { + cli::cli_abort( + c("Invalid input for parameter '{name}'", + i = 'Did you mean: {.strong "{as.character(parsed)}"} (in quotes)?', + i = explain_string), + call = call) + } + if (!is_simple_scalar_atomic(parsed)) { + cli::cli_abort( + c("Invalid input for parameter '{name}': {value}", + i = "Must be a simple boolean, number or string"), + call = call) + } + parsed[[1]] +} + + +get_missing_parameters_interactive <- function(required, envir, call = NULL) { + msg <- setdiff(required, names(envir)) + if (length(msg) == 0) { + return() + } + if (getOption("orderly_interactive_parameters_missing_error", FALSE)) { + cli::cli_abort( + c("Missing parameters: {squote(msg)}", + i = paste("Erroring because option", + "'orderly_interactive_parameters_missing_error'", + "is 'TRUE'")), + call = call) + } + n <- length(msg) + cli::cli_alert( + "Please enter values for {n} missing {cli::qty(n)}parameter{?s}:") + found <- list() + for (nm in msg) { + found[[nm]] <- get_parameter_interactive(nm, call) + } + list2env(found, envir) +} diff --git a/R/metadata.R b/R/metadata.R index 13536ca0..b81dbf83 100644 --- a/R/metadata.R +++ b/R/metadata.R @@ -44,6 +44,22 @@ static_orderly_strict_mode <- function(args) { ##' not come from a variable itself). Provide `NULL` if you do not ##' have a default, in which case this parameter will be required. ##' +##' @section Behaviour in interactive sessions: +##' +##' When running interactively (i.e., via `source()` or running an +##' `orderly.R` session by copy/paste or in Rstudio), the +##' `orderly_parameters()` function has different behaviour. +##' +##' First, we look in the current environment (most likely the global +##' environment) for values of your parameters - that is, variables +##' bound to the names of your parameters. For any parameters that +##' are not found we will look at the default values and use these +##' if possible, but if not possible then we will either error or +##' prompt based on the global option +##' `orderly_interactive_parameters_missing_error`. If this is +##' `TRUE`, then we will ask you to enter a value for the parameters +##' (strings will need to be entered with quotes). +##' ##' @title Declare orderly parameters ##' ##' @param ... Any number of parameters diff --git a/R/run.R b/R/run.R index e11d87e6..098296c0 100644 --- a/R/run.R +++ b/R/run.R @@ -408,19 +408,8 @@ check_parameters_interactive <- function(envir, spec, call) { return() } - is_required <- vlapply(spec, is.null) - - msg <- setdiff(names(spec)[is_required], names(envir)) - if (length(msg) > 0L) { - ## This will change, but we'll need some interactive prompting - ## better done in another ticket. See - ## https://github.com/r-lib/cli/issues/228 and - ## https://github.com/r-lib/cli/issues/488 for context here. - ## - ## There will be other "interactive mode" functions too that we'll - ## try and get a unified interface on. - stop("Missing parameters: ", paste(squote(msg), collapse = ", ")) - } + required <- names(spec)[vlapply(spec, is.null)] + get_missing_parameters_interactive(required, envir, call) ## Set any missing values into the environment: list2env(spec[setdiff(names(spec), names(envir))], envir) diff --git a/man/orderly_parameters.Rd b/man/orderly_parameters.Rd index 42c2d597..e8c32b21 100644 --- a/man/orderly_parameters.Rd +++ b/man/orderly_parameters.Rd @@ -21,3 +21,21 @@ boolean) and defaults must be present literally (i.e., they may not come from a variable itself). Provide \code{NULL} if you do not have a default, in which case this parameter will be required. } +\section{Behaviour in interactive sessions}{ + + +When running interactively (i.e., via \code{source()} or running an +\code{orderly.R} session by copy/paste or in Rstudio), the +\code{orderly_parameters()} function has different behaviour. + +First, we look in the current environment (most likely the global +environment) for values of your parameters - that is, variables +bound to the names of your parameters. For any parameters that +are not found we will look at the default values and use these +if possible, but if not possible then we will either error or +prompt based on the global option +\code{orderly_interactive_parameters_missing_error}. If this is +\code{TRUE}, then we will ask you to enter a value for the parameters +(strings will need to be entered with quotes). +} + diff --git a/tests/testthat/test-interactive.R b/tests/testthat/test-interactive.R index fc5171a1..1fb86052 100644 --- a/tests/testthat/test-interactive.R +++ b/tests/testthat/test-interactive.R @@ -12,3 +12,43 @@ test_that("can detect orderly directory", { root <- detect_orderly_interactive_path(file.path(path, "src", "explicit")) expect_equal(path, root) }) + + +test_that("can validate interactive parameters", { + mock_readline <- mockery::mock("TRUE", "100", "1.23", '"string"') + mockery::stub(get_parameter_interactive, "readline", mock_readline) + expect_equal(get_parameter_interactive("foo"), TRUE) + expect_equal(get_parameter_interactive("foo"), 100) + expect_equal(get_parameter_interactive("foo"), 1.23) + expect_equal(get_parameter_interactive("foo"), "string") +}) + + +test_that("can error when interactive parameters are invlid", { + mock_readline <- mockery::mock("", "hey ho", "string", "c(1, 2)") + mockery::stub(get_parameter_interactive, "readline", mock_readline) + err <- expect_error( + get_parameter_interactive("foo"), + "Expected a value for parameter 'foo'") + expect_null(err$body) + + err <- expect_error( + get_parameter_interactive("foo"), + "Failed to parse value for parameter 'foo'") + expect_length(err$body, 2) + expect_equal(err$body[1], c(x = "Was given: hey ho")) + expect_match(err$body[[2]], "If entering a string, you must use quotes") + + err <- expect_error( + get_parameter_interactive("foo"), + "Invalid input for parameter 'foo'") + expect_equal( + cli::ansi_strip(err$body[[1]]), + 'Did you mean: "string" (in quotes)?') + expect_match(err$body[[2]], "If entering a string, you must use quotes") + + err <- expect_error( + get_parameter_interactive("foo"), + "Invalid input for parameter 'foo'") + expect_equal(err$body[1], c(i = "Must be a simple boolean, number or string")) +}) diff --git a/tests/testthat/test-parameters.R b/tests/testthat/test-parameters.R index c9c182bb..9041da22 100644 --- a/tests/testthat/test-parameters.R +++ b/tests/testthat/test-parameters.R @@ -84,14 +84,45 @@ test_that("set defaults into environment if missing", { }) +test_that("fill in missing parameters", { + envir <- list2env(list(b = 3, c = 4), parent = new.env()) + mock_get <- mockery::mock({envir$a <- 1}) + mockery::stub(check_parameters_interactive, + "get_missing_parameters_interactive", + mock_get) + check_parameters_interactive(envir, list(a = NULL, b = NULL, c = NULL), NULL) + mockery::expect_called(mock_get, 1) + expect_equal(mockery::mock_args(mock_get)[[1]], + list(c("a", "b", "c"), envir, NULL)) + expect_mapequal(as.list(envir), list(a = 1, b = 3, c = 4)) +}) + + test_that("require non-default parameters are present in environment", { + withr::local_options(orderly_interactive_parameters_missing_error = TRUE) envir <- list2env(list(b = 3, c = 4), parent = new.env()) expect_error( - check_parameters_interactive(envir, list(a = NULL, b = NULL, c = NULL)), + get_missing_parameters_interactive(c("a", "b", "c"), envir), "Missing parameters: 'a'") }) +test_that("prompt for missing parameters", { + withr::local_options(orderly_interactive_parameters_missing_error = NULL) + envir <- list2env(list(c = 4), parent = new.env()) + mock_get <- mockery::mock(1, 2) + mockery::stub(get_missing_parameters_interactive, "get_parameter_interactive", + mock_get) + expect_message( + get_missing_parameters_interactive(c("a", "b", "c"), envir), + "Please enter values for 2 missing parameters:") + mockery::expect_called(mock_get, 2) + expect_equal(mockery::mock_args(mock_get), + list(list("a", NULL), list("b", NULL))) + expect_mapequal(as.list(envir), list(a = 1, b = 2, c = 4)) +}) + + test_that("parameters must be atomic scalars", { err <- expect_error( check_parameters(list(a = NULL, b = 2), list(a = NULL, b = NULL)),