New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

str_detect not implemented #120

Closed
danielsjf opened this Issue Aug 1, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@danielsjf

danielsjf commented Aug 1, 2016

str_detect('test','') 

Returns a not implemented error. More difficult to find when '' is hidden in a variable.

@hadley

This comment has been minimized.

Member

hadley commented Aug 2, 2016

What do you expect it to do?

@danielsjf

This comment has been minimized.

danielsjf commented Aug 2, 2016

The same as grepl(). Return TRUE if the string is a character (or even always TRUE).

@danielsjf

This comment has been minimized.

danielsjf commented Aug 2, 2016

Some examples

> grepl('','hello')
[1] TRUE
> grepl('',c('hello','x'))
[1] TRUE TRUE
> grepl('',c('hello','x',5))
[1] TRUE TRUE TRUE
> grepl('',7)
[1] TRUE

The last two are more a result of implicit casting to character of the string argument.

@hadley

This comment has been minimized.

Member

hadley commented Aug 2, 2016

It's more important for stringr to be internally consistent than to be consistent with base R functions. Currently stringr takes "" as a shortcut for boundary("character"), so I think the correct behaviour would be to return TRUE for strings that have >1 character.

@hadley

This comment has been minimized.

Member

hadley commented Aug 2, 2016

@gagolews do you have any thoughts the correct behaviour for "detecting" boundaries?

@gagolews

This comment has been minimized.

Contributor

gagolews commented Aug 10, 2016

IMO an error is a fine behavior in such a case.

problematic when '' is hidden in a variable -- The error message may be confusing:

> stringr::str_detect
function (string, pattern)                                                                                                           
{                                                                                                                                    
    switch(type(pattern), empty = , bound = stop("Not implemented",                                                                                                                          
        call. = FALSE), fixed = stri_detect_fixed(string, pattern,                                                                                                                           
        opts_fixed = attr(pattern, "options")), coll = stri_detect_coll(string,                                                                                                              
        pattern, opts_collator = attr(pattern, "options")), regex = stri_detect_regex(string,                                                                                                
        pattern, opts_regex = attr(pattern, "options")))                                                                                                                                     
}

why not replacing "Not implemented" with some more informative msg?

@hadley hadley closed this in 77249f5 Aug 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment