add sep argument to str_dup #564
Conversation
closes tidyverse#487 Adds a sep argument to str_dup. Becasue this is not supported by stri_dup, do separately and use lapply to go through strings.
R/dup.R
Outdated
| # stri_dup does not currently support sep | ||
| check_string(sep) | ||
| lapply(seq_along(string), function(i) { | ||
| if (length(times) == 1) { |
There was a problem hiding this comment.
I don't love how we have to condition on the length of times to handle vectorisation (i.e. when times is a scalar and string is a vector). Could we instead take the output from the vctrs::vec_size_common and use that?
There was a problem hiding this comment.
I have tried this but the result of vctrs::vec_size_common is the length of the string input, not the length of times
There was a problem hiding this comment.
Oh ooops needs to be vec_recycle_common()
R/dup.R
Outdated
| } else { | ||
| # stri_dup does not currently support sep | ||
| check_string(sep) | ||
| lapply(seq_along(string), function(i) { |
There was a problem hiding this comment.
I might be being a bit dim, but I couldn't manage to get this to work. The problem I was hitting was that map_chr wants to return a length 1 vector.
|
@hadley thanks for your review (and adding some nice comments in amongst the requested changes 😄). I have left a few comments above. |
| stri_dup(string, times) | ||
| str_dup <- function(string, times, sep = NULL) { | ||
| input <- vctrs::vec_recycle_common(string = string, times = times) | ||
| check_string(sep, allow_null = TRUE) |
There was a problem hiding this comment.
I think it makes sense to move the other input check up here so that all the input checks are together.
| @@ -13,3 +13,21 @@ test_that("0 duplicates equals empty string", { | |||
| test_that("uses tidyverse recycling rules", { | |||
| expect_error(str_dup(1:2, 1:3), class = "vctrs_error_incompatible_size") | |||
| }) | |||
|
|
|||
| test_that("uses sep argument", { | |||
| expect_equal(str_dup("abc", 1, sep = "-"), "abc") | |||
There was a problem hiding this comment.
I tried to make it a bit more clear was different in each test by keeping more stuff the same.
closes #487
Adds a sep argument to str_dup. Becasue this is not supported by stri_dup, do separately and use lapply to go through strings.