add allow_na argument to check character() #1742
Conversation
|
Although the error message that it appears now may not be very intuitive: 
|
R/standalone-types-check.R
Outdated
| if (is_character(x) & allow_na) { | ||
| return(invisible(NULL)) | ||
| } | ||
| if(is.character(x) & !(TRUE %in% is.na(x)) & !allow_na){ |
There was a problem hiding this comment.
These checks should use the scalar boolean operators, e.g. &&. (Also missing a space between if and (.)
There was a problem hiding this comment.
A simpler way of expressing TRUE %in% is.na(x) is any(is.na(x)).
There was a problem hiding this comment.
To solve the error message I think I'd structure this code like this:
if (is_character(x)) {
if (!allow_na && any(is.na(x))) {
abort(...)
}
return(invisible(NULL))
}With a specially crafted error message in the abort() call.
| expect_null(check_character(NULL, allow_null = TRUE)) | ||
| expect_error(check_character(NULL, allow_null = FALSE)) | ||
|
|
||
| expect_error(check_character(c("a",NA))) |
There was a problem hiding this comment.
This should be tested in the expect_snapshot() block below so the error message is tracked.
R/standalone-types-check.R
Outdated
| @@ -459,23 +459,34 @@ check_formula <- function(x, | |||
|
|
|||
| check_character <- function(x, | |||
| ..., | |||
| allow_na = FALSE, | |||
There was a problem hiding this comment.
This would be a breaking change here? The default should be TRUE imo. Because, at the moment, it always allows NA as suggests the issue title
lionel-
force-pushed
the
add-allowna-argument-to-check-character
branch
from
852f1a1
to
3e674b8
Compare
|
Thanks @martaalcalde! |
Addresses part of #1724