Glue collapse should always return a single string #295

hadley · 2023-01-26T23:49:46Z

Fixes #88

jennybc · 2023-03-13T22:24:04Z

I don't have a strong opinion on this (at least not yet). But it feels like a big-ish decision worth some discussion.

(I just kicked off cloud revdep checks for this branch, in case that reveals anything interesting.)

This doesn't feel consistent with tidyverse recycling rules. Specifically the one that says anything combined with something of length-0 becomes length-0:

tibble::tibble(x = integer(), y = 1L)
#> # A tibble: 0 × 2
#> # … with 2 variables: x <int>, y <int>
vctrs::vec_size_common(character())
#> [1] 0

Do we think that collapsing has nothing to do with tidyverse recycling rules and thus this doesn't matter?

Or do we think of collapsing as always having a "shadow" empty string "" as part of the collapsees?

jennybc · 2023-03-13T23:15:55Z

Revdep results:

We checked 614 reverse dependencies (608 from CRAN + 6 from Bioconductor), comparing R CMD check results across CRAN and dev versions of this package.

We saw 3 new problems
We failed to check 29 packages

These are the 3 packages with new problems: cpp11, lvmisc, tinkr.

The cpp11 result looks obviously related to this. ~~The other two will require a bit more analysis.~~ lvmisc and tinkr are also related to this PR. I have fixes in local PRs, which I will make into actual PRs once I merge this.

(Most of the packages that weren't checked are due to sf not getting installed.)

cpp11

Version: 0.4.3
GitHub: https://github.com/r-lib/cpp11
Source code: https://github.com/cran/cpp11
Date/Publication: 2022-10-12 08:43:54 UTC
Number of recursive dependencies: 81

Run revdepcheck::cloud_details(, "cpp11") for more info

Newly broken

checking tests ... ERROR

  Running ‘testthat.R’
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
   3.   └─base::seq.default(mid, end)
  ── Failure ('test-register.R:222:5'): generate_cpp_functions: returns the empty string if there are no functions ──
  generate_cpp_functions(funs) (`actual`) not equal to character() (`expected`).
  
  `actual`:   ""
  `expected`:   
  ── Failure ('test-register.R:391:5'): generate_r_functions: returns the empty string if there are no functions ──
  generate_r_functions(funs) (`actual`) not equal to character() (`expected`).
  
  `actual`:   ""
  `expected`:   
  
  [ FAIL 3 | WARN 0 | SKIP 2 | PASS 104 ]
  Error: Test failures
  Execution halted

lvmisc

Version: 0.1.1
GitHub: https://github.com/verasls/lvmisc
Source code: https://github.com/cran/lvmisc
Date/Publication: 2021-04-05 15:20:02 UTC
Number of recursive dependencies: 136

Run revdepcheck::cloud_details(, "lvmisc") for more info

Newly broken

checking tests ... ERROR

  Running ‘testthat.R’
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
  err$message (`actual`) not equal to glue::glue("The method `my_fun` is not yet implemented \\\n      for an object of class `my_class`.") (`expected`).
  
  lines(actual) vs lines(expected)
    "The method `my_fun` is not yet implemented for an object of class `my_class`."
  - ""
  ── Failure ('test-abort.R:109:5'): abort_no_method_for_class() works in objects with multiple classes ──
  err$message (`actual`) not equal to glue::glue("The method `my_fun` is not yet implemented \\\n       for an object of classes `my_class_1` and `my_class_2`.") (`expected`).
  
  lines(actual) vs lines(expected)
    "The method `my_fun` is not yet implemented for an object of classes `my_class_1` and `my_class_2`."
  - ""
  
  [ FAIL 2 | WARN 18 | SKIP 7 | PASS 238 ]
  Error: Test failures
  Execution halted

tinkr

Version: 0.1.0
GitHub: https://github.com/ropensci/tinkr
Source code: https://github.com/cran/tinkr
Date/Publication: 2022-08-19 12:30:02 UTC
Number of recursive dependencies: 62

Run revdepcheck::cloud_details(, "tinkr") for more info

Newly broken

checking tests ... ERROR

  Running ‘testthat.R’
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
  
  `lines(actual[[2]])` is a character vector ('## Dataset', '', 'The data used:', '[data](https://example.com)', '')
  `lines(expected[[2]])` is absent
  
  [ FAIL 14 | WARN 0 | SKIP 12 | PASS 92 ]
  Deleting unused snapshots:
  • class-yarn/yarn-kilroy.md
  • class-yarn/yarn.Rmd
  • class-yarn/yarn.md
  • to_md/new-code-chunk.Rmd
  • to_md/table.md
  • to_md/to_md-works-for-Rmd.Rmd
  • to_md/to_md-works.md
  Error: Test failures
  Execution halted

DavisVaughan · 2023-03-14T00:12:52Z

Just adding a quick note, not a full review yet:

I treat collapsing as a summary operation (i.e. ?Summary) like min()/max()/any()/all(), which always return a size 1 result regardless of their inputs. i.e. any(logical()) == FALSE.

So I feel like there is a strong support in favor of this change.

The docs also say

glue_collapse() collapses a character vector of any length into a length 1 vector

Regarding recycling rules, I don't think this is quite the same thing. It is somewhat adjacent, because both are about size stability, and I think always returning a size 1 result regardless of the input is an important part of that size stability.

Lastly, we may also consider an na_rm argument (since summary functions generally always have one) that removes missing values from x before collapsing.

jennybc · 2023-03-14T01:52:12Z

Noting also that this PR makes glue_collapse() work (more) like paste(collapse =).

paste(    collapse="|")
#> [1] ""
paste(    collapse="|", recycle0 = TRUE)
#> [1] ""
paste({}, collapse="|")
#> [1] ""
paste({}, collapse="|", recycle0 = TRUE)
#> [1] ""
paste(character(), collapse="|")
#> [1] ""
paste(character(), collapse="|", recycle0 = TRUE)
#> [1] ""

^{Created on 2023-03-13 with reprex v2.0.2.9000}

jennybc · 2023-03-14T01:54:16Z

Thanks @DavisVaughan.

I can certainly get on board with this being the right thing to do. In which case, I'll just need to follow up on any associated breakages (which look to be fairly modest).

jennybc · 2023-03-14T15:03:20Z

In particular, this is the sort of principle I was looking for. I.e. no the tidyverse recycling rules don't guide us here, but rather this convention around a summary does:

I treat collapsing as a summary operation (i.e. ?Summary) like min()/max()/any()/all(), which always return a size 1 result regardless of their inputs. i.e. any(logical()) == FALSE.

jennybc · 2023-03-14T23:04:46Z

In all 3 of cpp11, lvmisc, and tinkr, there is (at least) one function that basically makes a series of calls to glue_collapse() or, effectively, nested calls.

Therefore, it was actually kind of handy that length-0 inputs effectively disappeared.

With this PR, the fact that glue_collapse() returns "" for the "no input" case creates new, gratuitous newlines in cpp11, lvmisc, and tinkr.

Here's a super simplified version of the type of function I'm describing. Notice the trailing \n when I call foo() without providing any extra_junk via .... That trailing \n is not present if actual extra_junk is passed in.

library(glue)
packageVersion("glue")
#> [1] '1.6.2.9000'

foo <- function(main_stuff, ...) {
  extra_junk <- list(...)
  extra_junk <- glue_collapse(extra_junk, sep = "\n")
  
  glue_collapse(c(main_stuff, extra_junk), sep = "\n")
}

foo("Hello!", "and", "Goodbye!")
#> Hello!
#> and
#> Goodbye!

unclass(foo("Hello!", "and", "Goodbye!"))
#> [1] "Hello!\nand\nGoodbye!"

foo("Yo!")
#> Yo!

unclass(foo("Yo!"))
#> [1] "Yo!\n"

It makes me wonder if we should offer a way of telling glue_collapse() to drop length-0 inputs. Something along these lines:

input <- c("", "hey", "", "", "I herd", "", "you like", "whitespace")

glue_collapse(input, sep = "\n")
#> 
#> hey
#> 
#> 
#> I herd
#> 
#> you like
#> whitespace

glue_collapse(input[nzchar(input)], sep = "\n")
#> hey
#> I herd
#> you like
#> whitespace

To @DavisVaughan's point about na.rm, maybe this is the more relevant feature for glue_collapse()?

jennybc · 2023-03-15T00:26:39Z

I also note the extra newlines problem goes away if one does "combine then summarize" instead of "summarize, combine, then summarize again". But in real applications sometimes you really have to do the latter.

library(glue)
packageVersion("glue")
#> [1] '1.6.2.9000'

foo <- function(main_stuff, ...) {
  extra_junk <- list(...)
  extra_junk <- glue_collapse(extra_junk, sep = "\n")
  
  #glue_collapse(c(main_stuff, extra_junk), sep = "\n")
  glue_collapse(c(main_stuff, ...), sep = "\n")
}

foo("Hello!", "and", "Goodbye!")
#> Hello!
#> and
#> Goodbye!

unclass(foo("Hello!", "and", "Goodbye!"))
#> [1] "Hello!\nand\nGoodbye!"

foo("Yo!")
#> Yo!

unclass(foo("Yo!"))
#> [1] "Yo!"

hadley · 2023-03-15T11:35:26Z

This isn't exactly the same, but I think it's in the same ballpark: I see a lot of people write code like this:

paste0(c("Hello!", "and", "Goodbye!"), collapse = "\n")

But that's actually subtly wrong because it puts a new line in between each line, not at the end of each line. In most cases what I think you actually want is this:

paste0(c("Hello!", "and", "Goodbye!"), "\n", collapse = "")

So I don't find that example particularly compelling as a reason to break the very strong convention of "summary functions always return a scalar".

DavisVaughan · 2023-03-15T14:04:51Z

Somewhat motivating find: we actually have glue_collapse_data() that post processes the glue_collapse() result to force this size stability by returning "" in the empty case
https://github.com/r-lib/cpp11/blob/2ddc12f6ab268542c88f4aa24a500682ed260b72/R/utils.R#L1-L7

jennybc · 2023-03-15T17:06:36Z

So @hadley you're basically arguing the exact opposite of what I was seeing in those revdeps. I think it's because the affected glue_collapse() usages were mostly generating some sort of message, where that trailing newline doesn't need to be explicit. I.e. we're more likely to want message("howdy") than message("howdy\n").

But I'm going to merge this and make the relevant PRs elsewhere. Thanks for the discussion.

jennybc · 2023-03-15T17:30:08Z

So I don't find that example particularly compelling as a reason to break the very strong convention of "summary functions always return a scalar".

I wasn't proposing to not return a scalar. I was proposing some sort of compact = TRUE type of argument that says to drop empty strings, in the presence of at least 1 nonempty string.

* Respond to changes proposed in tidyverse/glue#295 * Use the glue PR * Remove glue remote, put back in Suggests (not Imports) * Skip empty string tests for insufficient glue version

hadley added 2 commits January 26, 2023 17:43

Return empty string from empty input to glue_collapse()

d4a61ad

Fixes #88

glue_sql_collapse() now always returns a single string

a0564f2

Fixes #88

hadley requested a review from jennybc January 26, 2023 23:49

Merge branch 'main' into empty-glue-collapse

5adfa17

jennybc requested a review from DavisVaughan March 13, 2023 22:24

jennybc added a commit to r-lib/cpp11 that referenced this pull request Mar 14, 2023

Respond to changes proposed in tidyverse/glue#295

9fa9a1e

jennybc added 2 commits March 14, 2023 09:38

Tweak NEWS

d24bb37

Sync up with @returns changes in other PRs

aa3fc3c

DavisVaughan approved these changes Mar 15, 2023

View reviewed changes

revdep check run: 7d1dd8d0-899e-4613-87eb-f93a8ecaa40d

ef376cc

jennybc merged commit cbac82a into main Mar 17, 2023

jennybc deleted the empty-glue-collapse branch March 17, 2023 05:32

This was referenced Mar 17, 2023

Respond to upcoming changes in glue::glue_collapse() r-lib/cpp11#310

Merged

If no extra_msg, don't append gratuitous \n verasls/lvmisc#3

Merged

If no yaml, avoid prepending with gratuitous \n ropensci/tinkr#93

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Glue collapse should always return a single string #295

Glue collapse should always return a single string #295

hadley commented Jan 26, 2023

jennybc commented Mar 13, 2023

jennybc commented Mar 13, 2023 •

edited

Loading

DavisVaughan commented Mar 14, 2023 •

edited

Loading

jennybc commented Mar 14, 2023

jennybc commented Mar 14, 2023

jennybc commented Mar 14, 2023 •

edited

Loading

jennybc commented Mar 14, 2023 •

edited

Loading

jennybc commented Mar 15, 2023

hadley commented Mar 15, 2023

DavisVaughan commented Mar 15, 2023

jennybc commented Mar 15, 2023 •

edited

Loading

jennybc commented Mar 15, 2023

Glue collapse should always return a single string #295

Glue collapse should always return a single string #295

Conversation

hadley commented Jan 26, 2023

jennybc commented Mar 13, 2023

jennybc commented Mar 13, 2023 • edited Loading

cpp11

Newly broken

lvmisc

Newly broken

tinkr

Newly broken

DavisVaughan commented Mar 14, 2023 • edited Loading

jennybc commented Mar 14, 2023

jennybc commented Mar 14, 2023

jennybc commented Mar 14, 2023 • edited Loading

jennybc commented Mar 14, 2023 • edited Loading

jennybc commented Mar 15, 2023

hadley commented Mar 15, 2023

DavisVaughan commented Mar 15, 2023

jennybc commented Mar 15, 2023 • edited Loading

jennybc commented Mar 15, 2023

jennybc commented Mar 13, 2023 •

edited

Loading

DavisVaughan commented Mar 14, 2023 •

edited

Loading

jennybc commented Mar 14, 2023 •

edited

Loading

jennybc commented Mar 14, 2023 •

edited

Loading

jennybc commented Mar 15, 2023 •

edited

Loading