Implement str_like() closes #280#315
Conversation
| fixed = stri_detect_fixed(string, pattern, negate = negate, opts_fixed = opts(pattern)), | ||
| coll = stri_detect_coll(string, pattern, negate = negate, opts_collator = opts(pattern)), | ||
| regex = { | ||
| pattern2 <- stri_replace_all_regex(pattern, "(?<!\\\\|\\[)%(?!\\])", "\\.\\*") |
There was a problem hiding this comment.
I'd recommend pulling this out into a separate function. Then it will be easier to test, and str_like() can become a thinner wrapper around str_detect()
|
Split the conversion into its own function and test set. |
hadley
left a comment
hadley
left a comment
There was a problem hiding this comment.
Looking good — a few more suggestions below.
| #' | ||
| #' @examples | ||
| #' like_converter("bana%") | ||
| #' like_converter("app_e") |
There was a problem hiding this comment.
You don't need to document this function because it's not exported; and generally I think it's better to put little helper functions like this at the bottom of the file (so you see the big picture first, before the details)
There was a problem hiding this comment.
And I think like_to_regex would be a slightly better name.
| #' str_like(fruit, "ba_ana") | ||
| #' str_like(fruit, "%APPLE") | ||
| str_like <- function(string, pattern, negate = FALSE, ignore_case = TRUE) { | ||
| switch(type(pattern), |
There was a problem hiding this comment.
I think you could now simplify this to:
if (type(pattern) == "regex") {
pattern <- like_converter(pattern)
}
str_detect(string, pattern, negate = negate, ignore_case = ignore_case)
Does that make sense?
There was a problem hiding this comment.
Hmmm, or should this function accept the other pattern types at all? Maybe that doesn't make sense for a more specialised function. In that case the function would look more like:
switch(type(pattern),
regex = stri_detect_regex(string, like_converter(pattern), negate = negate, opts_regex = opts(pattern)),
stop("str_like() works only with regular expressions", call. = FALSE
)There was a problem hiding this comment.
I agree that this should probably just accept a regex, have updated similar to above and made a separate call in order to preserve the ignore case earlier.
| }) | ||
|
|
||
| test_that("str_like works", { | ||
| expect_true(str_like("abc", "a_c")) |
There was a problem hiding this comment.
Once you've rewritten str_like() as I suggested above you'll be able to substantially simplify this test.
There was a problem hiding this comment.
Have kept only one of the general test for str_like() as we are testing the like_to_regex converter earlier. Do you think there is any further way to simplify/should there be a test to ensure it fails on any other type of regex now?
There was a problem hiding this comment.
I decided there was a slightly better way to check pattern so I tweaked the test too
|
Thanks for all your hard work on this! |
2 participants
Created a str_like() function to match the SQL operator's wild cards and escaping. Works by converting the SQL version to their regex equivalents.
Closes #280