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 for namespaced join_by() helpers #6951

Merged
merged 4 commits into from
Nov 6, 2023
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
3 changes: 3 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -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).

Expand Down
26 changes: 25 additions & 1 deletion R/join-by.R
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,22 @@ parse_join_by_expr <- function(expr, i, error_call) {
abort(message, call = error_call)
}

op <- as_string(expr[[1]])
if (is_call(expr, ns = "dplyr")) {
# Normalize by removing the `dplyr::`
DavisVaughan marked this conversation as resolved.
Show resolved Hide resolved
expr[[1]] <- sym(call_name(expr))
}

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,
Expand Down Expand Up @@ -476,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,
Expand Down
50 changes: 50 additions & 0 deletions tests/testthat/_snaps/join-by.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -125,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
Expand Down
24 changes: 24 additions & 0 deletions tests/testthat/test-join-by.R
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down Expand Up @@ -266,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))

Expand Down
Loading