stri_c should silently drop NULLs #116

Closed
hadley opened this Issue Nov 26, 2014 · 11 comments

Comments

Projects
None yet
2 participants
@hadley

hadley commented Nov 26, 2014

stringi::stri_c("a", NULL, "b")

I think that's useful because it allows you to write expressions like:

stringi::stri_c(
  "a",
  if (f()) " and b",
)
@gagolews

This comment has been minimized.

Show comment
Hide comment
@gagolews

gagolews Nov 26, 2014

Owner

I agree.

The behavior of stri_c below will also change then

> stringi::stri_c("a", character(0), "b")
character(0)
Owner

gagolews commented Nov 26, 2014

I agree.

The behavior of stri_c below will also change then

> stringi::stri_c("a", character(0), "b")
character(0)

@gagolews gagolews added this to the stringi-0.4 milestone Nov 26, 2014

@gagolews gagolews self-assigned this Nov 26, 2014

@hadley

This comment has been minimized.

Show comment
Hide comment
@hadley

hadley Nov 26, 2014

Yeah, I'm trying to think about that. I can't figure out which behaviour I prefer:

stringi::stri_c("a", letters[c(1, 2, 3)])
stringi::stri_c("a", letters[c(1, 2)])
stringi::stri_c("a", letters[c(1)])
stringi::stri_c("a", letters[c()])

stringr::str_c("a", letters[c(1, 2, 3)])
stringr::str_c("a", letters[c(1, 2)])
stringr::str_c("a", letters[c(1)])
stringr::str_c("a", letters[c()])

When I wrote stringr I wanted this behaviour (silently dropping empty strings), but I don't recall why :/ Maybe to make recycling more consistent? i.e. the output from str_c is always as long as the longest input.

hadley commented Nov 26, 2014

Yeah, I'm trying to think about that. I can't figure out which behaviour I prefer:

stringi::stri_c("a", letters[c(1, 2, 3)])
stringi::stri_c("a", letters[c(1, 2)])
stringi::stri_c("a", letters[c(1)])
stringi::stri_c("a", letters[c()])

stringr::str_c("a", letters[c(1, 2, 3)])
stringr::str_c("a", letters[c(1, 2)])
stringr::str_c("a", letters[c(1)])
stringr::str_c("a", letters[c()])

When I wrote stringr I wanted this behaviour (silently dropping empty strings), but I don't recall why :/ Maybe to make recycling more consistent? i.e. the output from str_c is always as long as the longest input.

@gagolews

This comment has been minimized.

Show comment
Hide comment
@gagolews

gagolews Nov 26, 2014

Owner

In case of all stringi functions, if one of the arguments is of length 0, then the result is always of zero length, something like:

> 1:10+c()
numeric(0)

This new stri_c behavior will violate this rule, but it's one of the few stringi functions that takes a ... arg.

Owner

gagolews commented Nov 26, 2014

In case of all stringi functions, if one of the arguments is of length 0, then the result is always of zero length, something like:

> 1:10+c()
numeric(0)

This new stri_c behavior will violate this rule, but it's one of the few stringi functions that takes a ... arg.

@gagolews

This comment has been minimized.

Show comment
Hide comment
@gagolews

gagolews Nov 26, 2014

Owner

BTW, we currently have one more stri_c-related issue to solve: #114

Owner

gagolews commented Nov 26, 2014

BTW, we currently have one more stri_c-related issue to solve: #114

@gagolews

This comment has been minimized.

Show comment
Hide comment
@gagolews

gagolews Nov 26, 2014

Owner

So maybe I'll add an additional ignore_null arg, that defaults to FALSE for backward compatibility?

Owner

gagolews commented Nov 26, 2014

So maybe I'll add an additional ignore_null arg, that defaults to FALSE for backward compatibility?

@hadley

This comment has been minimized.

Show comment
Hide comment
@hadley

hadley Nov 26, 2014

That seems reasonable to me.

hadley commented Nov 26, 2014

That seems reasonable to me.

@gagolews

This comment has been minimized.

Show comment
Hide comment
@gagolews

gagolews Nov 27, 2014

Owner

All right, almost done:

> stri_paste(NULL, c("a", "b"), sep=" ", ignore_null = TRUE)
[1] " a" " b"
> paste(NULL, c("a", "b"), sep=" ")
[1] " a" " b"
> stri_paste(NULL, sep=" ", ignore_null = TRUE)
character(0)
> paste(NULL, sep=" ")
character(0)
Owner

gagolews commented Nov 27, 2014

All right, almost done:

> stri_paste(NULL, c("a", "b"), sep=" ", ignore_null = TRUE)
[1] " a" " b"
> paste(NULL, c("a", "b"), sep=" ")
[1] " a" " b"
> stri_paste(NULL, sep=" ", ignore_null = TRUE)
character(0)
> paste(NULL, sep=" ")
character(0)

gagolews added a commit that referenced this issue Nov 27, 2014

@gagolews gagolews closed this Nov 27, 2014

@hadley

This comment has been minimized.

Show comment
Hide comment
@hadley

hadley Nov 27, 2014

This doesn't behave quite how I expect:

expect_equal(str_c(test, NULL, "a", sep = " "), c("a a", "b a", "c a"))

I think if NULLs are dropped, their corresponding separator should be too.

hadley commented Nov 27, 2014

This doesn't behave quite how I expect:

expect_equal(str_c(test, NULL, "a", sep = " "), c("a a", "b a", "c a"))

I think if NULLs are dropped, their corresponding separator should be too.

@gagolews gagolews reopened this Nov 27, 2014

@gagolews

This comment has been minimized.

Show comment
Hide comment
@gagolews

gagolews Nov 27, 2014

Owner

OK, now I get what you need. It's done.

Owner

gagolews commented Nov 27, 2014

OK, now I get what you need. It's done.

gagolews added a commit that referenced this issue Nov 27, 2014

@gagolews gagolews closed this Nov 27, 2014

@hadley

This comment has been minimized.

Show comment
Hide comment
@hadley

hadley Nov 27, 2014

BTW do you know you can close issues from commit messages with e.g. close #116 ?

hadley commented Nov 27, 2014

BTW do you know you can close issues from commit messages with e.g. close #116 ?

@gagolews

This comment has been minimized.

Show comment
Hide comment
@gagolews

gagolews Nov 27, 2014

Owner

👍

Owner

gagolews commented Nov 27, 2014

👍

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