From e0badb64676bcb08b95a231226ddf090c5861873 Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Thu, 18 Jan 2024 11:41:03 +0000 Subject: [PATCH 1/8] Improve error message when using orderly interactively. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is quite easy and common for users to open up a file in RStudio and start running commands from it interactively, using Ctrl-Enter, without setting their working directory to match that file. When that happens, orderly would throw an unhelpful error, saying only `Failed to detect orderly path: /path/to/workspace` without any mention of working directories, or that the path should be a report directory (as opposed to eg. the top-level orderly workspace). The error message is changed to look along the lines of: ``` ! Working directory /path/to/workspace is not a valid orderly report. ``` This should be more helpful, by making it clear what is incorrect (the working directory), and what was expected (an orderly report). Additionally, if running in RStudio and the current active editor is a file in what looks like an orderly report, it will augment the error message with a suggestion of what `setwd` command to run: ``` ! Working directory /path/to/workspace is not a valid orderly report. ℹ Use `setwd("/path/to/workspace/src/my-report")` to set the working directory to the report currently open in RStudio. ``` Finally, even if the working directory is a valid report, but it does not match the directory of the currently active editor (eg. the user has switched from one report to another), orderly commands will succeed as before, but will show a warning to the user: ``` Working directory /path/to/workspace/src/my-report does not match the report currently open in RStudio. ℹ Use `setwd("/path/to/workspace/src/other-report")` to switch working directories. ``` Frustratingly, while RStudio supports "click-to-run" snippets of code in its output, it restricts the syntax of these and in particular does not allow methods from the base package, such as `setwd`. Instead the user has to copy-paste the suggestion into the command prompt themselves. See https://github.com/rstudio/rstudio/issues/11273 and https://github.com/rstudio/rstudio/pull/11434. The restriction is pretty arbitrary and we could easily workaround it by re-exporting the `setwd` function in the orderly2 package if we really wanted to. --- DESCRIPTION | 1 + R/interactive.R | 52 +++++++++++++++++++++++---- R/util.R | 9 +++++ tests/testthat/helper-orderly.R | 2 +- tests/testthat/test-interactive.R | 58 ++++++++++++++++++++++++++----- 5 files changed, 105 insertions(+), 17 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index dc4ddc87..0f9fbdd6 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -22,6 +22,7 @@ Imports: jsonlite, openssl, rlang, + rstudioapi, withr, yaml Suggests: diff --git a/R/interactive.R b/R/interactive.R index e97200f6..1d078cfb 100644 --- a/R/interactive.R +++ b/R/interactive.R @@ -1,16 +1,54 @@ +is_plausible_orderly_report <- function(path) { + path_split <- fs::path_split(path)[[1]] + + length(path_split) > 2 && + path_split[[length(path_split) - 1]] == "src" && + file.exists(file.path(path, "../..", "orderly_config.yml")) +} + +rstudio_get_current_active_editor_path <- function() { + if (rstudioapi::isAvailable()) { + rstudioapi::getSourceEditorContext()$path + } else { + NULL + } +} + ## This is something that we might improve over time - it will likely ## be useful to have some sort of "register interactive" function ## which we could then just look up. ## ## I am not sure if we also want to allow working interactively from a ## draft directory too. -detect_orderly_interactive_path <- function(path = getwd()) { - path_split <- fs::path_split(path)[[1]] - is_plausible <- length(path_split) > 2 && - path_split[[length(path_split) - 1]] == "src" && - file.exists(file.path(path, "../..", "orderly_config.yml")) - if (!is_plausible) { - stop(sprintf("Failed to detect orderly path at '%s'", path)) +detect_orderly_interactive_path <- function(path = getwd(), + editor_path = rstudio_get_current_active_editor_path()) { + is_valid <- is_plausible_orderly_report(path) + suggested_wd <- NULL + if (!is.null(editor_path)) { + dir <- fs::path_dir(editor_path) + if (paths_are_different(dir, path) && is_plausible_orderly_report(dir)) { + suggested_wd <- dir + } + } + + if (!is_plausible_orderly_report(path)) { + msg <- c("Working directory {.path {path}} is not a valid orderly report.") + if (!is.null(suggested_wd)) { + cli::cli_abort( + c(msg, i = "Use {.code setwd({.str {suggested_wd}})} to set the working directory to the report currently open in RStudio.")) + } else { + cli::cli_abort(msg) + } + } + + if (!is.null(suggested_wd)) { + # For some reason, cli_warn has a different behaviour than cli_abort and + # doesn't make individual bullet points available in the condition object. + # The following mimicks cli_abort more closely, making testing easier. + msg <- c( + "Working directory {.path {path}} does not match the report currently open in RStudio.", + i = "Use {.code setwd({.str {suggested_wd}})} to switch working directories.") + rlang::warn(vcapply(msg, cli::format_inline), use_cli_format = TRUE) } as.character(fs::path_norm(file.path(path, "../.."))) } diff --git a/R/util.R b/R/util.R index 0c9bdcdc..5faa86c2 100644 --- a/R/util.R +++ b/R/util.R @@ -632,3 +632,12 @@ saverds_atomic <- function(data, path, allow_fail = FALSE) { finally = unlink(tmp)) } } + + +paths_are_different <- function(x, y) { + tryCatch({ + x_real <- fs::path_real(x) + y_real <- fs::path_real(y) + x_real != y_real + }, error = function(e) FALSE) +} diff --git a/tests/testthat/helper-orderly.R b/tests/testthat/helper-orderly.R index 9ce5646d..48cd9f84 100644 --- a/tests/testthat/helper-orderly.R +++ b/tests/testthat/helper-orderly.R @@ -1,6 +1,6 @@ options(outpack.schema_validate = requireNamespace("jsonvalidate", quietly = TRUE) && - packageVersion("jsonvalidate") >= "1.4.0", + utils::packageVersion("jsonvalidate") >= "1.4.0", orderly_index_progress = FALSE) diff --git a/tests/testthat/test-interactive.R b/tests/testthat/test-interactive.R index 1fb86052..1d46063d 100644 --- a/tests/testthat/test-interactive.R +++ b/tests/testthat/test-interactive.R @@ -1,18 +1,58 @@ test_that("can detect orderly directory", { - path <- test_prepare_orderly_example("explicit") - envir <- new.env() - id <- orderly_run_quietly("explicit", root = path, envir = envir) + root <- test_prepare_orderly_example("explicit") + detected_root <- detect_orderly_interactive_path(file.path(root, "src", "explicit")) + expect_equal(detected_root) +}) + +test_that("errors when working directory is not report", { + root <- test_prepare_orderly_example("explicit") expect_error( - detect_orderly_interactive_path(path), - "Failed to detect orderly path at") + detect_orderly_interactive_path(root), + "Working directory .* is not a valid orderly report.") + expect_error( - detect_orderly_interactive_path(file.path(path, "src")), - "Failed to detect orderly path at") - root <- detect_orderly_interactive_path(file.path(path, "src", "explicit")) - expect_equal(path, root) + detect_orderly_interactive_path(file.path(root, "src")), + "Working directory .* is not a valid orderly report.") +}) + +test_that("suggests changing working directory", { + # Make matching simpler by avoiding line-wrapping. + withr::local_options(cli.width=Inf) + + root <- test_prepare_orderly_example(c("explicit", "implicit")) + + e <- expect_error(detect_orderly_interactive_path( + path = file.path(root, "src"), + editor_path = file.path(root, "src", "implicit", "orderly.R")), + "Working directory .* is not a valid orderly report") + expect_match(e$body[[1]], "Use `setwd(.*)` to set the working directory to the report currently open in RStudio") + + w <- expect_warning(detect_orderly_interactive_path( + path = file.path(root, "src", "explicit"), + editor_path = file.path(root, "src", "implicit", "orderly.R")), + "Working directory .* does not match the report currently open in RStudio") + expect_match(w$body[[1]], "Use `setwd(.*)` to switch working directories") }) +test_that("does not unnecessarily suggest changing working directory", { + root <- test_prepare_orderly_example("explicit") + + expect_no_warning(detect_orderly_interactive_path( + path = file.path(root, "src", "explicit"), + editor_path = "Untitled" + )) + + expect_no_warning(detect_orderly_interactive_path( + path = file.path(root, "src", "explicit"), + editor_path = file.path(root, "src", "explicit", "orderly.R") + )) + + expect_no_warning(detect_orderly_interactive_path( + path = file.path(root, "src", "explicit"), + editor_path = file.path(root, "orderly_config.yml") + )) +}) test_that("can validate interactive parameters", { mock_readline <- mockery::mock("TRUE", "100", "1.23", '"string"') From 5bf3ea96e665be8e04aec6de88f41a30faf076fb Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Thu, 18 Jan 2024 12:16:25 +0000 Subject: [PATCH 2/8] Fix formatting issues --- tests/testthat/test-interactive.R | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/testthat/test-interactive.R b/tests/testthat/test-interactive.R index 1d46063d..60d0bfd1 100644 --- a/tests/testthat/test-interactive.R +++ b/tests/testthat/test-interactive.R @@ -1,6 +1,7 @@ test_that("can detect orderly directory", { root <- test_prepare_orderly_example("explicit") - detected_root <- detect_orderly_interactive_path(file.path(root, "src", "explicit")) + detected_root <- detect_orderly_interactive_path( + file.path(root, "src", "explicit")) expect_equal(detected_root) }) @@ -18,7 +19,7 @@ test_that("errors when working directory is not report", { test_that("suggests changing working directory", { # Make matching simpler by avoiding line-wrapping. - withr::local_options(cli.width=Inf) + withr::local_options(cli.width = Inf) root <- test_prepare_orderly_example(c("explicit", "implicit")) From e540d01d275e94b681c91b8bf0f5f5225ba4157d Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Thu, 18 Jan 2024 12:26:32 +0000 Subject: [PATCH 3/8] Fix bad test --- R/interactive.R | 6 +++--- tests/testthat/test-interactive.R | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/R/interactive.R b/R/interactive.R index 1d078cfb..332ef7dd 100644 --- a/R/interactive.R +++ b/R/interactive.R @@ -46,9 +46,9 @@ detect_orderly_interactive_path <- function(path = getwd(), # doesn't make individual bullet points available in the condition object. # The following mimicks cli_abort more closely, making testing easier. msg <- c( - "Working directory {.path {path}} does not match the report currently open in RStudio.", - i = "Use {.code setwd({.str {suggested_wd}})} to switch working directories.") - rlang::warn(vcapply(msg, cli::format_inline), use_cli_format = TRUE) + cli::format_inline("Working directory {.path {path}} does not match the report currently open in RStudio."), + cli::format_inline(i = "Use {.code setwd({.str {suggested_wd}})} to switch working directories.")) + rlang::warn(msg, use_cli_format = TRUE) } as.character(fs::path_norm(file.path(path, "../.."))) } diff --git a/tests/testthat/test-interactive.R b/tests/testthat/test-interactive.R index 60d0bfd1..3f07056a 100644 --- a/tests/testthat/test-interactive.R +++ b/tests/testthat/test-interactive.R @@ -2,7 +2,7 @@ test_that("can detect orderly directory", { root <- test_prepare_orderly_example("explicit") detected_root <- detect_orderly_interactive_path( file.path(root, "src", "explicit")) - expect_equal(detected_root) + expect_equal(detected_root, root) }) test_that("errors when working directory is not report", { From 0876d74b4b429cda474c7d5738c49cd1a8f9e4c0 Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Thu, 18 Jan 2024 12:34:41 +0000 Subject: [PATCH 4/8] Avoid looking at the RStudio state when running tests inside of it. --- R/interactive.R | 3 ++- R/util.R | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/R/interactive.R b/R/interactive.R index 332ef7dd..98d9ea30 100644 --- a/R/interactive.R +++ b/R/interactive.R @@ -7,7 +7,8 @@ is_plausible_orderly_report <- function(path) { } rstudio_get_current_active_editor_path <- function() { - if (rstudioapi::isAvailable()) { + # Avoid looking at the RStudio state when running tests inside of it. + if (!is_testing() && rstudioapi::isAvailable()) { rstudioapi::getSourceEditorContext()$path } else { NULL diff --git a/R/util.R b/R/util.R index 5faa86c2..e5b41785 100644 --- a/R/util.R +++ b/R/util.R @@ -641,3 +641,10 @@ paths_are_different <- function(x, y) { x_real != y_real }, error = function(e) FALSE) } + + +is_testing <- function() { + # Copied from testthat, to avoid having the package as a run-time dependency. + # https://github.com/r-lib/testthat/blob/fe50a222c62cc8733b397690caf3b2a95856f902/R/test-env.R#L20 + identical(Sys.getenv("TESTTHAT"), "true") +} From 51c927fe1a093c30909506a911c1c2cb4ddedc1f Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Fri, 19 Jan 2024 13:48:33 +0000 Subject: [PATCH 5/8] Code review. --- DESCRIPTION | 2 +- R/interactive.R | 21 ++++++++++++++------- R/util.R | 8 ++------ tests/testthat/helper-orderly.R | 2 +- 4 files changed, 18 insertions(+), 15 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 0f9fbdd6..fa203067 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: orderly2 Title: Orderly Next Generation -Version: 1.99.10 +Version: 1.99.11 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 98d9ea30..57d78c3f 100644 --- a/R/interactive.R +++ b/R/interactive.R @@ -21,13 +21,15 @@ rstudio_get_current_active_editor_path <- function() { ## ## I am not sure if we also want to allow working interactively from a ## draft directory too. -detect_orderly_interactive_path <- function(path = getwd(), - editor_path = rstudio_get_current_active_editor_path()) { +detect_orderly_interactive_path <- function( + path = getwd(), + editor_path = rstudio_get_current_active_editor_path()) +{ is_valid <- is_plausible_orderly_report(path) suggested_wd <- NULL if (!is.null(editor_path)) { dir <- fs::path_dir(editor_path) - if (paths_are_different(dir, path) && is_plausible_orderly_report(dir)) { + if (!paths_are_identical(dir, path) && is_plausible_orderly_report(dir)) { suggested_wd <- dir } } @@ -35,8 +37,9 @@ detect_orderly_interactive_path <- function(path = getwd(), if (!is_plausible_orderly_report(path)) { msg <- c("Working directory {.path {path}} is not a valid orderly report.") if (!is.null(suggested_wd)) { - cli::cli_abort( - c(msg, i = "Use {.code setwd({.str {suggested_wd}})} to set the working directory to the report currently open in RStudio.")) + cli::cli_abort(c(msg, i = paste( + "Use {.code setwd({.str {suggested_wd}})} to set the working", + "directory to the report currently open in RStudio."))) } else { cli::cli_abort(msg) } @@ -47,8 +50,12 @@ detect_orderly_interactive_path <- function(path = getwd(), # doesn't make individual bullet points available in the condition object. # The following mimicks cli_abort more closely, making testing easier. msg <- c( - cli::format_inline("Working directory {.path {path}} does not match the report currently open in RStudio."), - cli::format_inline(i = "Use {.code setwd({.str {suggested_wd}})} to switch working directories.")) + cli::format_inline(paste( + "Working directory {.path {path}} does not match the report currently", + "open in RStudio.")), + cli::format_inline(i = paste( + "Use {.code setwd({.str {suggested_wd}})}", + "to switch working directories."))) rlang::warn(msg, use_cli_format = TRUE) } as.character(fs::path_norm(file.path(path, "../.."))) diff --git a/R/util.R b/R/util.R index e5b41785..7b9640fc 100644 --- a/R/util.R +++ b/R/util.R @@ -634,12 +634,8 @@ saverds_atomic <- function(data, path, allow_fail = FALSE) { } -paths_are_different <- function(x, y) { - tryCatch({ - x_real <- fs::path_real(x) - y_real <- fs::path_real(y) - x_real != y_real - }, error = function(e) FALSE) +paths_are_identical <- function(x, y) { + fs::path_norm(x) == fs::path_norm(y) } diff --git a/tests/testthat/helper-orderly.R b/tests/testthat/helper-orderly.R index 48cd9f84..9ce5646d 100644 --- a/tests/testthat/helper-orderly.R +++ b/tests/testthat/helper-orderly.R @@ -1,6 +1,6 @@ options(outpack.schema_validate = requireNamespace("jsonvalidate", quietly = TRUE) && - utils::packageVersion("jsonvalidate") >= "1.4.0", + packageVersion("jsonvalidate") >= "1.4.0", orderly_index_progress = FALSE) From 4374ba28ceeee869e37502a684f7c36ab713c550 Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Mon, 22 Jan 2024 17:32:50 +0000 Subject: [PATCH 6/8] Minor improvements. --- R/interactive.R | 3 ++- tests/testthat/test-interactive.R | 16 +++++++++------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/R/interactive.R b/R/interactive.R index 57d78c3f..a6e26618 100644 --- a/R/interactive.R +++ b/R/interactive.R @@ -49,11 +49,12 @@ detect_orderly_interactive_path <- function( # For some reason, cli_warn has a different behaviour than cli_abort and # doesn't make individual bullet points available in the condition object. # The following mimicks cli_abort more closely, making testing easier. + # https://github.com/r-lib/cli/issues/666 msg <- c( cli::format_inline(paste( "Working directory {.path {path}} does not match the report currently", "open in RStudio.")), - cli::format_inline(i = paste( + i=cli::format_inline(paste( "Use {.code setwd({.str {suggested_wd}})}", "to switch working directories."))) rlang::warn(msg, use_cli_format = TRUE) diff --git a/tests/testthat/test-interactive.R b/tests/testthat/test-interactive.R index 3f07056a..ab6fe6b4 100644 --- a/tests/testthat/test-interactive.R +++ b/tests/testthat/test-interactive.R @@ -18,16 +18,15 @@ test_that("errors when working directory is not report", { }) test_that("suggests changing working directory", { - # Make matching simpler by avoiding line-wrapping. - withr::local_options(cli.width = Inf) - root <- test_prepare_orderly_example(c("explicit", "implicit")) e <- expect_error(detect_orderly_interactive_path( path = file.path(root, "src"), editor_path = file.path(root, "src", "implicit", "orderly.R")), "Working directory .* is not a valid orderly report") - expect_match(e$body[[1]], "Use `setwd(.*)` to set the working directory to the report currently open in RStudio") + expect_match(e$body[[1]], paste( + "Use `setwd(.*)` to set the working directory", + "to the report currently open in RStudio")) w <- expect_warning(detect_orderly_interactive_path( path = file.path(root, "src", "explicit"), @@ -39,19 +38,22 @@ test_that("suggests changing working directory", { test_that("does not unnecessarily suggest changing working directory", { root <- test_prepare_orderly_example("explicit") + # Editor path is already the current working directory expect_no_warning(detect_orderly_interactive_path( path = file.path(root, "src", "explicit"), - editor_path = "Untitled" + editor_path = file.path(root, "src", "explicit", "orderly.R") )) + # Editor path is not an orderly report expect_no_warning(detect_orderly_interactive_path( path = file.path(root, "src", "explicit"), - editor_path = file.path(root, "src", "explicit", "orderly.R") + editor_path = file.path(root, "orderly_config.yml") )) + # Editor path is an unsaved file expect_no_warning(detect_orderly_interactive_path( path = file.path(root, "src", "explicit"), - editor_path = file.path(root, "orderly_config.yml") + editor_path = "Untitled" )) }) From a9287be16877b193f68a4556ba4efba7ec998aec Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Mon, 22 Jan 2024 17:47:56 +0000 Subject: [PATCH 7/8] Format fix --- R/interactive.R | 5 ++--- R/util.R | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/R/interactive.R b/R/interactive.R index a6e26618..e30a814b 100644 --- a/R/interactive.R +++ b/R/interactive.R @@ -23,8 +23,7 @@ rstudio_get_current_active_editor_path <- function() { ## draft directory too. detect_orderly_interactive_path <- function( path = getwd(), - editor_path = rstudio_get_current_active_editor_path()) -{ + editor_path = rstudio_get_current_active_editor_path()) { is_valid <- is_plausible_orderly_report(path) suggested_wd <- NULL if (!is.null(editor_path)) { @@ -54,7 +53,7 @@ detect_orderly_interactive_path <- function( cli::format_inline(paste( "Working directory {.path {path}} does not match the report currently", "open in RStudio.")), - i=cli::format_inline(paste( + i = cli::format_inline(paste( "Use {.code setwd({.str {suggested_wd}})}", "to switch working directories."))) rlang::warn(msg, use_cli_format = TRUE) diff --git a/R/util.R b/R/util.R index 7b9640fc..4178fb8b 100644 --- a/R/util.R +++ b/R/util.R @@ -641,6 +641,6 @@ paths_are_identical <- function(x, y) { is_testing <- function() { # Copied from testthat, to avoid having the package as a run-time dependency. - # https://github.com/r-lib/testthat/blob/fe50a222c62cc8733b397690caf3b2a95856f902/R/test-env.R#L20 + # https://github.com/r-lib/testthat/blob/fe50a22/R/test-env.R#L20 identical(Sys.getenv("TESTTHAT"), "true") } From b6080ef7e5678e986521ec0f29937b7a00daf47c Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Mon, 22 Jan 2024 18:18:20 +0000 Subject: [PATCH 8/8] Add mock-based test of RStudio API --- tests/testthat/test-interactive.R | 38 +++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/testthat/test-interactive.R b/tests/testthat/test-interactive.R index ab6fe6b4..96315a70 100644 --- a/tests/testthat/test-interactive.R +++ b/tests/testthat/test-interactive.R @@ -57,6 +57,44 @@ test_that("does not unnecessarily suggest changing working directory", { )) }) +test_that("rstudio API is not called when unavailable", { + testthat::skip_if_not_installed("mockery") + mock_rstudio_available <- mockery::mock(FALSE) + mock_rstudio_context <- mockery::mock() + mockery::stub( + rstudio_get_current_active_editor_path, + "is_testing", + mockery::mock(FALSE)) + mockery::stub( + rstudio_get_current_active_editor_path, + "rstudioapi::isAvailable", + mock_rstudio_available) + mockery::stub( + rstudio_get_current_active_editor_path, + "rstudioapi::getSourceEditorContext", + mockery::mock(FALSE)) + expect_null(rstudio_get_current_active_editor_path()) + mockery::expect_called(mock_rstudio_available, 1) + mockery::expect_called(mock_rstudio_context, 0) +}) + +test_that("rstudio API is used to find current editor path", { + testthat::skip_if_not_installed("mockery") + mockery::stub( + rstudio_get_current_active_editor_path, + "is_testing", + mockery::mock(FALSE)) + mockery::stub( + rstudio_get_current_active_editor_path, + "rstudioapi::isAvailable", + mockery::mock(TRUE)) + mockery::stub( + rstudio_get_current_active_editor_path, + "rstudioapi::getSourceEditorContext", + mockery::mock(list(path = "/path/to/file"))) + expect_equal(rstudio_get_current_active_editor_path(), "/path/to/file") +}) + test_that("can validate interactive parameters", { mock_readline <- mockery::mock("TRUE", "100", "1.23", '"string"') mockery::stub(get_parameter_interactive, "readline", mock_readline)