From 3f0ee308878efc5d6e0788438056e1fcd3a12363 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Fri, 3 Nov 2023 11:20:42 -0400 Subject: [PATCH 1/4] Allow for namespaced `join_by()` helpers --- R/join-by.R | 7 +++++++ tests/testthat/_snaps/join-by.md | 32 ++++++++++++++++++++++++++++++++ tests/testthat/test-join-by.R | 17 +++++++++++++++++ 3 files changed, 56 insertions(+) diff --git a/R/join-by.R b/R/join-by.R index d4a1c395ee..e0ce8c23ab 100644 --- a/R/join-by.R +++ b/R/join-by.R @@ -431,6 +431,13 @@ parse_join_by_expr <- function(expr, i, error_call) { abort(message, call = error_call) } + if (is_string(call_ns(expr), "dplyr")) { + # Normalize by removing the `dplyr::` + fn <- call_name(expr) + args <- call_args(expr) + expr <- call2(fn, !!!args) + } + op <- as_string(expr[[1]]) switch( diff --git a/tests/testthat/_snaps/join-by.md b/tests/testthat/_snaps/join-by.md index 5e8e5a5fed..2b2770c4b3 100644 --- a/tests/testthat/_snaps/join-by.md +++ b/tests/testthat/_snaps/join-by.md @@ -16,6 +16,38 @@ ! Expressions using `==` can't contain missing arguments. x Argument `y` is missing. +# allows for namespaced helpers (#6838) + + Code + join_by(dplyr::between(x, left, right)) + Output + Join By: + - dplyr::between(x, left, right) + +--- + + Code + join_by(dplyr::within(xl, xu, yl, yu)) + Output + Join By: + - dplyr::within(xl, xu, yl, yu) + +--- + + Code + join_by(dplyr::overlaps(xl, xu, yl, yu)) + Output + Join By: + - dplyr::overlaps(xl, xu, yl, yu) + +--- + + Code + join_by(dplyr::closest(x < y)) + Output + Join By: + - dplyr::closest(x < y) + # has an informative print method Code diff --git a/tests/testthat/test-join-by.R b/tests/testthat/test-join-by.R index afbcfacf8f..b885468668 100644 --- a/tests/testthat/test-join-by.R +++ b/tests/testthat/test-join-by.R @@ -236,6 +236,23 @@ test_that("nicely catches required missing arguments when wrapped", { expect_snapshot(error = TRUE, fn(a)) }) +test_that("allows for namespaced helpers (#6838)", { + # Captures namespaced expression for printing + expect_snapshot(join_by(dplyr::between(x, left, right))) + expect_snapshot(join_by(dplyr::within(xl, xu, yl, yu))) + expect_snapshot(join_by(dplyr::overlaps(xl, xu, yl, yu))) + expect_snapshot(join_by(dplyr::closest(x < y))) + + # Underlying values are otherwise the same as non-namespaced version + by <- join_by(dplyr::between(x, left, right)) + reference <- join_by(between(x, left, right)) + + expect_identical(by$condition, reference$condition) + expect_identical(by$filter, reference$filter) + expect_identical(by$x, reference$x) + expect_identical(by$y, reference$y) +}) + test_that("has an informative print method", { expect_snapshot(join_by(a, b)) expect_snapshot(join_by("a", "b")) From 5bbe9e9d0dc8b9283ccdcc9761254699920c0976 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Fri, 3 Nov 2023 11:21:38 -0400 Subject: [PATCH 2/4] NEWS bullet --- NEWS.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS.md b/NEWS.md index ce17cd42b0..88ac3aff11 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # dplyr (development version) +* `join_by()` now allows its helper functions to be namespaced with `dplyr::`, + like `join_by(dplyr::between(x, lower, upper))` (#6838). + * `dplyr_reconstruct()`'s default method has been rewritten to avoid materializing duckplyr queries too early (#6947). From b51d43290c8eca65ccca60a5669f76e1e02283ee Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Mon, 6 Nov 2023 08:48:25 -0500 Subject: [PATCH 3/4] Simplify from code review --- R/join-by.R | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/R/join-by.R b/R/join-by.R index e0ce8c23ab..4018822953 100644 --- a/R/join-by.R +++ b/R/join-by.R @@ -431,11 +431,9 @@ parse_join_by_expr <- function(expr, i, error_call) { abort(message, call = error_call) } - if (is_string(call_ns(expr), "dplyr")) { + if (is_call(expr, ns = "dplyr")) { # Normalize by removing the `dplyr::` - fn <- call_name(expr) - args <- call_args(expr) - expr <- call2(fn, !!!args) + expr[[1]] <- sym(call_name(expr)) } op <- as_string(expr[[1]]) From a1255fe10cfbf96eb2676a3e8ff74c1135412bf9 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Mon, 6 Nov 2023 09:35:02 -0500 Subject: [PATCH 4/4] Improve error handling for typos and odd calls --- R/join-by.R | 21 ++++++++++++++++++++- tests/testthat/_snaps/join-by.md | 18 ++++++++++++++++++ tests/testthat/test-join-by.R | 7 +++++++ 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/R/join-by.R b/R/join-by.R index 4018822953..d02ff7035f 100644 --- a/R/join-by.R +++ b/R/join-by.R @@ -436,7 +436,17 @@ parse_join_by_expr <- function(expr, i, error_call) { expr[[1]] <- sym(call_name(expr)) } - op <- as_string(expr[[1]]) + op <- expr[[1]] + + if (!is_symbol(op)) { + if (is_call(op, name = "::")) { + stop_invalid_namespaced_expression(expr, i, error_call) + } else { + stop_invalid_top_expression(expr, i, error_call) + } + } + + op <- as_string(op) switch( op, @@ -481,6 +491,15 @@ stop_invalid_top_expression <- function(expr, i, call) { abort(message, call = call) } +stop_invalid_namespaced_expression <- function(expr, i, call) { + message <- c( + glue("Expressions can only be namespace prefixed with `dplyr::`."), + i = glue("Expression {i} is {err_expr(expr)}.") + ) + + abort(message, call = call) +} + parse_join_by_name <- function(expr, i, default_side, diff --git a/tests/testthat/_snaps/join-by.md b/tests/testthat/_snaps/join-by.md index 2b2770c4b3..0ae8873aad 100644 --- a/tests/testthat/_snaps/join-by.md +++ b/tests/testthat/_snaps/join-by.md @@ -157,6 +157,24 @@ ! Each element of `...` must be a single column name or a join by expression. x Element 1 is not a name and not an expression. +--- + + Code + join_by(1()) + Condition + Error in `join_by()`: + ! Expressions must use one of: `==`, `>=`, `>`, `<=`, `<`, `closest()`, `between()`, `overlaps()`, or `within()`. + i Expression 1 is `1()`. + +--- + + Code + join_by(dplyrr::between(x, left, right)) + Condition + Error in `join_by()`: + ! Expressions can only be namespace prefixed with `dplyr::`. + i Expression 1 is `dplyrr::between(x, left, right)`. + --- Code diff --git a/tests/testthat/test-join-by.R b/tests/testthat/test-join-by.R index b885468668..8ddb875661 100644 --- a/tests/testthat/test-join-by.R +++ b/tests/testthat/test-join-by.R @@ -283,6 +283,13 @@ test_that("has informative error messages", { # Garbage input expect_snapshot(error = TRUE, join_by(1)) + # Call with non-symbol first element + expect_snapshot(error = TRUE, join_by(1())) + + # Namespace prefixed helper with non-dplyr namespace + # (typo or re-export, which currently isn't allowed) + expect_snapshot(error = TRUE, join_by(dplyrr::between(x, left, right))) + # Top level usage of `$` expect_snapshot(error = TRUE, join_by(x$a))