Skip to content

Commit

Permalink
fix in ard_stack() when calls to functions were namespaced (#250)
Browse files Browse the repository at this point in the history
**What changes are proposed in this pull request?**
* Bug fix in `ard_stack()` when calls to functions were namespaced.
(#242)

**Reference GitHub issue associated with pull request.** _e.g., 'closes
#<issue number>'_
closes #242 

@bzkrouse have you come across a method for testing if a function works
if the package is not loaded? That could be a good test to add too.


--------------------------------------------------------------------------------

Pre-review Checklist (if item does not apply, mark is as complete)
- [ ] **All** GitHub Action workflows pass with a ✅
- [ ] PR branch has pulled the most recent updates from master branch:
`usethis::pr_merge_main()`
- [ ] If a bug was fixed, a unit test was added.
- [ ] Code coverage is suitable for any new functions/features
(generally, 100% coverage for new code): `devtools::test_coverage()`
- [ ] Request a reviewer

Reviewer Checklist (if item does not apply, mark is as complete)

- [ ] If a bug was fixed, a unit test was added.
- [ ] Run `pkgdown::build_site()`. Check the R console for errors, and
review the rendered website.
- [ ] Code coverage is suitable for any new functions/features:
`devtools::test_coverage()`

When the branch is ready to be merged:
- [ ] Update `NEWS.md` with the changes from this pull request under the
heading "`# cards (development version)`". If there is an issue
associated with the pull request, reference it in parentheses at the end
update (see `NEWS.md` for examples).
- [ ] **All** GitHub Action workflows pass with a ✅
- [ ] Approve Pull Request
- [ ] Merge the PR. Please use "Squash and merge" or "Rebase and merge".
  • Loading branch information
ddsjoberg authored May 15, 2024
1 parent 2dfbb38 commit d2a79a9
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 5 deletions.
2 changes: 2 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

* The `ard_stack(by)` argument has been renamed to `".by"` and its location moved to after the dots inputs, e.g. `ard_stack(..., .by)`. (#243)

* Bug fix in `ard_stack()` when calls to functions were namespaced. (#242)

* Added `check_ard_structure(column_order, method)` arguments to the function to check for column ordering and whether result contains a `stat_name='method'` row.

* Converting `ard_*()` functions and other helpers to S3 generics to make them extendable. (#227)
Expand Down
24 changes: 19 additions & 5 deletions R/ard_stack.R
Original file line number Diff line number Diff line change
Expand Up @@ -169,11 +169,25 @@ ard_stack <- function(data,
lapply(
dots,
function(x) {
x_rhs <- f_rhs(x)
x_fn <- call_name(x_rhs)
x_args <- call_args(x_rhs)

do.call(x_fn, c(list(data = data, by = .by), x_args), envir = attr(x, ".Environment"))
if (!is_call_simple(x)) {
cli::cli_abort(
"{.fun cards::ard_stack} works with {.help [simple calls](rlang::call_name)}
and {.code {as_label(x)}} is not simple.",
call = get_cli_abort_call()
)
}
x_ns <- call_ns(x)
x_fn <- call_name(x)
x_args <- call_args(x)

# if a function was namespaced, then grab function from that pkg's Namespace
# styler: off
final_fn <-
if (is.null(x_ns)) x_fn
else get(x_fn, envir = asNamespace(x_ns))
# styler: on

do.call(final_fn, c(list(data = data, by = .by), x_args), envir = attr(x, ".Environment"))
}
)
}
1 change: 1 addition & 0 deletions inst/WORDLIST
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ httr
jsonlite
mis
nalysis
namespaced
nonmiss
pre
quosures
Expand Down
11 changes: 11 additions & 0 deletions tests/testthat/_snaps/ard_stack.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,14 @@
Message
i 2 more variables: warning, error

# ard_stack() complex call error

Code
complex_call <- list()
complex_call$ard_continuous <- ard_continuous
ard_stack(data = mtcars, .by = am, complex_call$ard_continuous(variables = "mpg"),
)
Condition
Error in `ard_stack()`:
! `cards::ard_stack()` works with simple calls (`?rlang::call_name()`) and `complex_call$ard_continuous(variables = "mpg")` is not simple.

29 changes: 29 additions & 0 deletions tests/testthat/test-ard_stack.R
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,20 @@ test_that("ard_stack() .shuffle argument", {
)
})


test_that("ard_stack() works with namespaced functions", {
expect_equal(
ard_stack(
data = mtcars,
cards::ard_continuous(variables = "mpg")
),
ard_stack(
data = mtcars,
ard_continuous(variables = "mpg")
)
)
})

test_that("ard_stack() messaging", {
expect_snapshot(
ard_stack(
Expand All @@ -200,3 +214,18 @@ test_that("ard_stack() messaging", {
head(1L)
)
})

test_that("ard_stack() complex call error", {
expect_snapshot(
{
complex_call <- list()
complex_call$ard_continuous <- ard_continuous
ard_stack(
data = mtcars,
.by = am,
complex_call$ard_continuous(variables = "mpg"),
)
},
error = TRUE
)
})

0 comments on commit d2a79a9

Please sign in to comment.