Skip to content

Commit

Permalink
Added check for protected names in ard_categorical.survey.design() (#…
Browse files Browse the repository at this point in the history
…182)

**What changes are proposed in this pull request?**
* Adding a check for variables named `variable` and `variable_level` in
the `ard_categorical(by)` argument.

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


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

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.
- [ ] If a new `ard_*()` function was added, it passes the ARD
structural checks from `cards::check_ard_structure()`.
- [ ] If a new `ard_*()` function was added, `set_cli_abort_call()` has
been set.
- [ ] If a new `ard_*()` function was added and it depends on another
package (such as, `broom`), `is_pkg_installed("broom", reference_pkg =
"cardx")` has been set in the function call and the following added to
the roxygen comments: `@examplesIf
do.call(asNamespace("cardx")$is_pkg_installed, list(pkg = "broom"",
reference_pkg = "cardx"))`
- [ ] Code coverage is suitable for any new functions/features
(generally, 100% coverage for new code): `devtools::test_coverage()`

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

- [ ] If a bug was fixed, a unit test was added.
- [ ] 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 "`# cardx (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 Jul 8, 2024
1 parent 0f7b271 commit 7f9ed34
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 0 deletions.
9 changes: 9 additions & 0 deletions R/ard_categorical.survey.design.R
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,15 @@ ard_categorical.survey.design <- function(data,
}
)

# return note about column names that result in errors -----------------------
if (any(by %in% c("variable", "variable_level"))) {
cli::cli_abort(
"The {.arg by} argument cannot include variables named {.val {c('variable', 'variable_level')}}.",
call = get_cli_abort_call()
)
}


# calculate counts -----------------------------------------------------------
# this tabulation accounts for unobserved combinations
svytable_counts <- .svytable_counts(data, variables, by, denominator)
Expand Down
8 changes: 8 additions & 0 deletions tests/testthat/_snaps/ard_categorical.survey.design.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,11 @@
! Column "Class" is all missing and cannot be tabulated.
i Only columns of class <logical> and <factor> can be tabulated when all values are missing.

# ard_categorical.survey.design(by) messages about protected names

Code
ard_categorical(svy_mtcars, by = variable, variables = gear)
Condition
Error in `ard_categorical()`:
! The `by` argument cannot include variables named "variable" and "variable_level".

26 changes: 26 additions & 0 deletions tests/testthat/test-ard_categorical.survey.design.R
Original file line number Diff line number Diff line change
Expand Up @@ -1172,3 +1172,29 @@ test_that("ard_categorical.survey.design() works with variables with only 1 leve
)
expect_invisible(cards::check_ard_structure(ard_svy_cat_cell, method = FALSE))
})


test_that("ard_categorical.survey.design(by) messages about protected names", {
svy_mtcars <-
survey::svydesign(
ids = ~1,
data = mtcars |>
dplyr::mutate(
variable = am,
variable_level = cyl,
by = am,
by_level = cyl
),
weights = ~1
)

expect_snapshot(
error = TRUE,
ard_categorical(svy_mtcars, by = variable, variables = gear)
)

expect_error(
ard_categorical(svy_mtcars, by = variable_level, variables = gear),
'The `by` argument cannot include variables named "variable" and "variable_level".'
)
})

0 comments on commit 7f9ed34

Please sign in to comment.