Additional check functions #86
Conversation
| cl <- match.call() | ||
| arg <- as.character(cl$x) | ||
|
|
||
| for (i in x) { |
There was a problem hiding this comment.
The downside here is that we only catch one violation at a time.
There was a problem hiding this comment.
I would imagine there's a pretty steep performance penalty here, too. This is the same kind of performance issue that led to tidymodels/parsnip#835.
Maybe an error could be conditional on rlang::is_integerish()?
There was a problem hiding this comment.
Is it steep for checking arguments (not data)? We need some sort of (short length) numeric vector checks with min/max/na/null options.
| ) | ||
| } | ||
|
|
||
| # check_logical <- function(x, |
There was a problem hiding this comment.
brulee has its own check_logical(). The is temporary to avoid bringing in a lot of snapshots into this PR.
tests/testthat/test-checkers.R
Outdated
| expect_silent(brulee:::check_number_whole_vec(variable)) | ||
| expect_silent(brulee:::check_number_whole_vec(variable[1])) | ||
|
|
||
| variable <- seq(0, 1, length.out = 3) |
There was a problem hiding this comment.
This one demonstrates that you can get a whole number returned that is not an integer data type.
| cl <- match.call() | ||
| arg <- as.character(cl$x) | ||
|
|
||
| for (i in x) { |
There was a problem hiding this comment.
I would imagine there's a pretty steep performance penalty here, too. This is the same kind of performance issue that led to tidymodels/parsnip#835.
Maybe an error could be conditional on rlang::is_integerish()?
| invisible(x) | ||
| } | ||
|
|
||
| check_number_decimal_vec <- function(x, call = rlang::caller_env(), ...) { |
tests/testthat/test-checkers.R
Outdated
| library(rlang) | ||
|
|
||
| variable <- "pie" | ||
| expect_snapshot(brulee:::check_single_logical(variable), error = TRUE) |
There was a problem hiding this comment.
| expect_snapshot(brulee:::check_single_logical(variable), error = TRUE) | |
| expect_snapshot(check_single_logical(variable), error = TRUE) |
In general, don't need to namespace functions from the package being tested.
There was a problem hiding this comment.
That was left over from testing (since we can source these standalone files)
tests/testthat/test-checkers.R
Outdated
There was a problem hiding this comment.
Could you rename either this file or the corresponding R file so that they have the same slug? I.e. either checks or checkers but not both.
tests/testthat/test-checkers.R
Outdated
| @@ -0,0 +1,50 @@ | |||
| test_that("checking single logicals", { | |||
| library(rlang) | |||
There was a problem hiding this comment.
What do we need this library() call for? Could it be removed? (Same comment for the other occurrences of this pattern.)
tests/testthat/test-checkers.R
Outdated
| variable <- "pie" | ||
| expect_snapshot(brulee:::check_single_logical(variable), error = TRUE) |
There was a problem hiding this comment.
The inputs to the check function here and below are so short, I would inline them into the call inside of expect_snapshot() so that we can see it in the snapshot .md file. That makes reviewing those easier.
There was a problem hiding this comment.
I want to make sure that the call includes variable and not the value of the argument. I added one exception here deliberately just to snapshot the in-lined variable (data).
There was a problem hiding this comment.
Ah, I see! We could combine both ideas like this:
| variable <- "pie" | |
| expect_snapshot(brulee:::check_single_logical(variable), error = TRUE) | |
| expect_snapshot(error = TRUE, { | |
| variable <- "pie" | |
| brulee:::check_single_logical(variable) | |
| }) |
tests/testthat/test-checkers.R
Outdated
| expect_snapshot(brulee:::check_single_logical(variable), error = TRUE) | ||
|
|
||
| variable <- NA | ||
| expect_snapshot(brulee:::check_single_logical(variable), error = TRUE) |
There was a problem hiding this comment.
I don't see a need to namespace the check function. Could we remove the brulee:: prefix for readability?
R/checks.R
Outdated
| check_number_whole_vec <- function(x, call = rlang::caller_env(), ...) { | ||
| cl <- match.call() | ||
| arg <- as.character(cl$x) |
There was a problem hiding this comment.
| check_number_whole_vec <- function(x, call = rlang::caller_env(), ...) { | |
| cl <- match.call() | |
| arg <- as.character(cl$x) | |
| check_number_whole_vec <- function(x, arg = rlang::caller_arg(x), call = rlang::caller_env(), ...) { |
|
Oh, we reviewed both! 👀 |
|
But at least we didn't contradict each other 😂 |
Co-authored-by: Simon P. Couch <simonpatrickcouch@gmail.com>
|
Worth noting, that a |
Co-authored-by: Emil Hvitfeldt <emilhhvitfeldt@gmail.com>
The rlang type checkers lack a few functions. This PR has three: one for a single vector and two to check numeric vectors.