Skip to content

Implement .null argument#217

Merged
jimhester merged 3 commits intotidyverse:masterfrom
echasnovski:null-arg-84
Oct 6, 2021
Merged

Implement .null argument#217
jimhester merged 3 commits intotidyverse:masterfrom
echasnovski:null-arg-84

Conversation

@echasnovski
Copy link
Copy Markdown
Contributor

This PR add .null argument to glue() and glue_data(). As it was suggested in comment, this fixes #84.

@jimhester
Copy link
Copy Markdown
Collaborator

Thanks for working on this!

However the PR is not currently quite the behavior we want.

If .null = NULL it should work like paste0() does with NULL inputs, e.g. they are silently dropped.

paste0("foo", NULL, "bar")
#> [1] "foobar"

If .null = character() it should work how glue does currently, e.g. return a 0 length vector.

I think we have also decided we are going to change the default behavior of glue() to the NULL case, as it makes doing things like glue::glue("foo{bar}", bar = if(x == 1) "y") nicer.

- If `character()`: return `character()` if there is any `NULL` element.
- If `NULL`: drop any `NULL` elements (as `paste0()`).
- Else: replace `NULL` with `.null`.
@echasnovski
Copy link
Copy Markdown
Contributor Author

echasnovski commented May 11, 2021

Added described behavior but didn't change the default value to NULL, because I am not sure that you want to do this now.

This introduces one peculiar edge case. What should be the output of glue("{NULL}", .null = NULL)? The output of paste0(NULL) is character(), but in this case I would expect the output to be "" because "NULL is inside quotes".

Currently I decided to stick with ".null = NULL emulates paste0()" because the "NULL inside quotes results into """ can be achieved with glue("{NULL}", .null = "").

@echasnovski
Copy link
Copy Markdown
Contributor Author

Also a question about a code I stumbled here. Is there a need for length(unnamed_args) < sum(!named) test? I removed this check, and no test got broken.

Git history shows that it was altered for two reasons: in bcfaf72 to account for lazy loading and in 9cb52dd to fix when last expression is NULL. However, lapply() returns output with the same length as input and which(!named) should have the same length as sum(!named) (as named seems to always be logical vector). Am I missing something here?

@jimhester
Copy link
Copy Markdown
Collaborator

It seems the test cases for that PR were not added when it was committed manually.

https://github.com/tidyverse/glue/pull/46/files#diff-dfe1d2c4b81146f1b57d333a16c3436162380561ea424fb6141aa71c978ee97aR245-R246

Could you add them and verify removing the conditional does not break the behavior?

@echasnovski
Copy link
Copy Markdown
Contributor Author

Yep, they are present. Removing that check doesn't break anything, both on master and in this branch (as long as .null = character() is default).

I'll remove this check here.

Relevant tests are in "length 0 inputs produce length 0 outputs", ones
which have `NULL` values. They don't break if check is removed.
@jimhester jimhester merged commit 943b66c into tidyverse:master Oct 6, 2021
@jimhester
Copy link
Copy Markdown
Collaborator

Thanks for working on this!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Distinguish between NULL and character()

2 participants