Remove stringi dependency #936 #986
Conversation
R/extract.R
Outdated
| @@ -55,7 +55,9 @@ str_extract <- function(x, into, regex, convert = FALSE) { | |||
| is_character(into) | |||
| ) | |||
|
|
|||
| matches <- stringi::stri_match_first_regex(x, regex)[, -1, drop = FALSE] | |||
| matches <- lapply(regmatches(x, regexec(regex, x)), | |||
There was a problem hiding this comment.
Could you please pull this out into a separate function? It's going to need unit tests for edge cases that stri_match_first_regex() might handle, but the base methods do not. I don't know exactly what those are, but I'd start with tests around no matches, NA matches, ...
There was a problem hiding this comment.
Pulled it out and added a test suite. Based it off of stringi and adjusted to match. stringi had a lot of tests that worked around capture groups and blanks however tidyr won't need it as it checks earlier in extract to ensure the number of capture groups is consistent with the into value
Co-authored-by: Hadley Wickham <[email protected]>
R/extract.R
Outdated
|
|
||
| str_match_first <- function(x, regex) { | ||
| if (length(x) == 0) { | ||
| # Can't determine number of matches |
There was a problem hiding this comment.
I think this needs a little extra thought (I added the branch just as a placeholder:
stringi::stri_match_first(character(), regex = "(.)-(.)")
#> [,1] [,2] [,3]
tidyr:::str_match_first(character(), regex = "(.)-(.)")
#> NULLThere was a problem hiding this comment.
Hmmmm, also problematic when there are no matches:
str(stringi::stri_match_first("", regex = "(.)-(.)"))
#> chr [1, 1:3] NA NA NA
str(tidyr:::str_match_first("", regex = "(.)-(.)"))
#> chr[1, 0 ]There was a problem hiding this comment.
Just remembered I have some code lying around for exactly this problem 😀
|
Thanks @rjpat! |
Closes #936
Have tried to completely sub out the stringi functions with the equivalent base ones and work without modifying any existing tests.