From a4d18d94a9d1d89d5352827949a34da23b702a93 Mon Sep 17 00:00:00 2001 From: ndelcamp Date: Thu, 15 Aug 2024 14:31:29 -0400 Subject: [PATCH 1/6] Make type.character error for NA_character_ Fixes #546 --- R/modifiers.R | 16 ++++++++++++++++ tests/testthat/_snaps/modifiers.md | 16 ++++++++++++++++ tests/testthat/test-modifiers.R | 12 ++++++++++++ 3 files changed, 44 insertions(+) diff --git a/R/modifiers.R b/R/modifiers.R index d183ac0b..34e425fa 100644 --- a/R/modifiers.R +++ b/R/modifiers.R @@ -202,6 +202,22 @@ type.stringr_fixed <- function(x, error_call = caller_env()) { } #' @export type.character <- function(x, error_call = caller_env()) { + if (length(x) == 1) { + if(is.na(x)) { + cli::cli_abort( + tr_("{.arg pattern} must be a string, not {.obj_type_friendly {x}}."), + call = error_call + ) + } + } + + if (any(is.na(x))) { + cli::cli_abort( + tr_("{.arg pattern} must be a string that does not contain NAs."), + call = error_call + ) + } + if (identical(x, "")) "empty" else "regex" } diff --git a/tests/testthat/_snaps/modifiers.md b/tests/testthat/_snaps/modifiers.md index 5d5e3307..8f809439 100644 --- a/tests/testthat/_snaps/modifiers.md +++ b/tests/testthat/_snaps/modifiers.md @@ -24,3 +24,19 @@ Error: ! `pattern` must be a string, not an integer vector. +# useful error message for NA_character_ + + Code + type(NA_character_) + Condition + Error: + ! `pattern` must be a string, not a character `NA`. + +# useful error message for vector that includes NAs + + Code + type(c("a", "b", NA_character_, "c")) + Condition + Error: + ! `pattern` must be a string that does not contain NAs. + diff --git a/tests/testthat/test-modifiers.R b/tests/testthat/test-modifiers.R index 34495494..40b74f4e 100644 --- a/tests/testthat/test-modifiers.R +++ b/tests/testthat/test-modifiers.R @@ -36,3 +36,15 @@ test_that("subsetting preserves class and options", { x <- regex("a", multiline = TRUE) expect_equal(x[], x) }) + +test_that("useful error message for NA_character_", { + expect_snapshot(error = TRUE, { + type(NA_character_) + }) +}) + +test_that("useful error message for vector that includes NAs", { + expect_snapshot(error = TRUE, { + type(c("a", "b", NA_character_, "c")) + }) +}) From 99ddec886c4fcf825940b9ddb042826d33f2b4da Mon Sep 17 00:00:00 2001 From: Nash Delcamp <107637296+nash-delcamp-slp@users.noreply.github.com> Date: Thu, 15 Aug 2024 17:51:16 -0400 Subject: [PATCH 2/6] Improve error language Co-authored-by: Hadley Wickham --- R/modifiers.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/modifiers.R b/R/modifiers.R index 34e425fa..5a0b0006 100644 --- a/R/modifiers.R +++ b/R/modifiers.R @@ -213,7 +213,7 @@ type.character <- function(x, error_call = caller_env()) { if (any(is.na(x))) { cli::cli_abort( - tr_("{.arg pattern} must be a string that does not contain NAs."), + tr_("{.arg pattern} must be a character vector that does not contain NAs."), call = error_call ) } From ce3a84277aa12246b78d098c33ba29fbe9c69ec1 Mon Sep 17 00:00:00 2001 From: ndelcamp Date: Thu, 15 Aug 2024 18:09:51 -0400 Subject: [PATCH 3/6] simplify NA check in `type()`; add news --- NEWS.md | 2 ++ R/modifiers.R | 9 --------- tests/testthat/_snaps/modifiers.md | 10 +--------- tests/testthat/test-modifiers.R | 6 ------ 4 files changed, 3 insertions(+), 24 deletions(-) diff --git a/NEWS.md b/NEWS.md index 365fd68b..5df570c4 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,7 @@ # stringr (development version) +* `str_remove()` and `str_remove_all()` now throw an error when `pattern` includes any `NA` values, including `NA_character_` (@nash-delcamp-slp, #546). + * In `str_replace_all()`, a `replacement` function now receives all values in a single vector. This radically improves performance at the cost of breaking some existing uses (#462). diff --git a/R/modifiers.R b/R/modifiers.R index 5a0b0006..ff9d52d8 100644 --- a/R/modifiers.R +++ b/R/modifiers.R @@ -202,15 +202,6 @@ type.stringr_fixed <- function(x, error_call = caller_env()) { } #' @export type.character <- function(x, error_call = caller_env()) { - if (length(x) == 1) { - if(is.na(x)) { - cli::cli_abort( - tr_("{.arg pattern} must be a string, not {.obj_type_friendly {x}}."), - call = error_call - ) - } - } - if (any(is.na(x))) { cli::cli_abort( tr_("{.arg pattern} must be a character vector that does not contain NAs."), diff --git a/tests/testthat/_snaps/modifiers.md b/tests/testthat/_snaps/modifiers.md index 8f809439..c215868e 100644 --- a/tests/testthat/_snaps/modifiers.md +++ b/tests/testthat/_snaps/modifiers.md @@ -24,19 +24,11 @@ Error: ! `pattern` must be a string, not an integer vector. -# useful error message for NA_character_ - - Code - type(NA_character_) - Condition - Error: - ! `pattern` must be a string, not a character `NA`. - # useful error message for vector that includes NAs Code type(c("a", "b", NA_character_, "c")) Condition Error: - ! `pattern` must be a string that does not contain NAs. + ! `pattern` must be a character vector that does not contain NAs. diff --git a/tests/testthat/test-modifiers.R b/tests/testthat/test-modifiers.R index 40b74f4e..efa6ef37 100644 --- a/tests/testthat/test-modifiers.R +++ b/tests/testthat/test-modifiers.R @@ -37,12 +37,6 @@ test_that("subsetting preserves class and options", { expect_equal(x[], x) }) -test_that("useful error message for NA_character_", { - expect_snapshot(error = TRUE, { - type(NA_character_) - }) -}) - test_that("useful error message for vector that includes NAs", { expect_snapshot(error = TRUE, { type(c("a", "b", NA_character_, "c")) From 7b977967851b0100cdeeef0f247fcdd2e789d20e Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 20 Aug 2024 14:33:41 -0500 Subject: [PATCH 4/6] Polish news --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 54e93d88..201544c4 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,6 +1,6 @@ # stringr (development version) -* `str_remove()` and `str_remove_all()` now throw an error when `pattern` includes any `NA` values, including `NA_character_` (@nash-delcamp-slp, #546). +* `str_*` now error if `pattern` includes any `NA`s (@nash-delcamp-slp, #546). * Adds `[[.stringr_pattern` method to go along with existing `[.stringr_pattern` method (@edward-burn, #569). * In `str_replace_all()`, a `replacement` function now receives all values in From 4f656184c745e09c3cc38c3c54b1d36221d51455 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 20 Aug 2024 14:34:26 -0500 Subject: [PATCH 5/6] Add NA test --- tests/testthat/_snaps/modifiers.md | 7 ++++++- tests/testthat/test-modifiers.R | 3 ++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/testthat/_snaps/modifiers.md b/tests/testthat/_snaps/modifiers.md index c215868e..fbc78c4d 100644 --- a/tests/testthat/_snaps/modifiers.md +++ b/tests/testthat/_snaps/modifiers.md @@ -24,8 +24,13 @@ Error: ! `pattern` must be a string, not an integer vector. -# useful error message for vector that includes NAs +# useful errors for NAs + Code + type(NA) + Condition + Error: + ! `pattern` must be a string, not `NA`. Code type(c("a", "b", NA_character_, "c")) Condition diff --git a/tests/testthat/test-modifiers.R b/tests/testthat/test-modifiers.R index 31729332..74e2765e 100644 --- a/tests/testthat/test-modifiers.R +++ b/tests/testthat/test-modifiers.R @@ -37,8 +37,9 @@ test_that("subsetting preserves class and options", { expect_equal(x[], x) }) -test_that("useful error message for vector that includes NAs", { +test_that("useful errors for NAs", { expect_snapshot(error = TRUE, { + type(NA) type(c("a", "b", NA_character_, "c")) }) }) From 514af157713ac5bb8f20b3f7705d66c49296fe7c Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 20 Aug 2024 14:35:42 -0500 Subject: [PATCH 6/6] Make message consistent --- R/modifiers.R | 2 +- tests/testthat/_snaps/modifiers.md | 4 ++-- tests/testthat/_snaps/split.md | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/R/modifiers.R b/R/modifiers.R index 1e339f17..f5afb6f4 100644 --- a/R/modifiers.R +++ b/R/modifiers.R @@ -220,7 +220,7 @@ type.default <- function(x, error_call = caller_env()) { } cli::cli_abort( - tr_("{.arg pattern} must be a string, not {.obj_type_friendly {x}}."), + tr_("{.arg pattern} must be a character vector, not {.obj_type_friendly {x}}."), call = error_call ) } diff --git a/tests/testthat/_snaps/modifiers.md b/tests/testthat/_snaps/modifiers.md index fbc78c4d..67ae93f8 100644 --- a/tests/testthat/_snaps/modifiers.md +++ b/tests/testthat/_snaps/modifiers.md @@ -22,7 +22,7 @@ type(1:3) Condition Error: - ! `pattern` must be a string, not an integer vector. + ! `pattern` must be a character vector, not an integer vector. # useful errors for NAs @@ -30,7 +30,7 @@ type(NA) Condition Error: - ! `pattern` must be a string, not `NA`. + ! `pattern` must be a character vector, not `NA`. Code type(c("a", "b", NA_character_, "c")) Condition diff --git a/tests/testthat/_snaps/split.md b/tests/testthat/_snaps/split.md index 574b9f8c..6b00f942 100644 --- a/tests/testthat/_snaps/split.md +++ b/tests/testthat/_snaps/split.md @@ -9,7 +9,7 @@ str_split("x", 1) Condition Error in `str_split()`: - ! `pattern` must be a string, not a number. + ! `pattern` must be a character vector, not a number. Code str_split("x", "x", n = 0) Condition