Refine + method (#309)

* Refine `+` method

* Add a test

* Update tests/testthat/test-glue.R

Co-authored-by: Hadley Wickham <h.wickham@gmail.com>

* Add a test for `+`ing size incompatible inputs

* Tidy up these tests

* Treat `NULL` like `character()` in `+`

This is different from what's planned for `glue_data()` / `glue()` in #246, but I think it's what we want.

In any case, it preserves existing behaviour. If we want something else, it would need to happen in #246, possibly as part of an edition.

* Tweak NEWS

---------

Co-authored-by: Hadley Wickham <h.wickham@gmail.com>

NEWS.md

@@ -2,7 +2,7 @@
 
 * `+` now works in more situations, and gives errors when one side isn't a 
   character vector. It no longer automatically applies glue interpolation to
-  both sides; you'll need to do that yourself as needed (#286).
+  a non-glue input, if there is one. You'll need to do that yourself (#286).
 
 * `glue_collapse(character())` (and hence `glue_sql_collapse(character())`) now
   return `""`, so that they always return a single string (#88).

R/glue.R

@@ -354,14 +354,18 @@ as.character.glue <- function(x, ...) {
 
 #' @export
 `+.glue` <- function(e1, e2) {
-  if (!is.character(e1)) {
+  if (!is.null(e1) && !is.character(e1)) {
     stop("LHS must be a character vector.")
   }
-  if (!is.character(e2)) {
+  if (!is.null(e2) && !is.character(e2)) {
     stop("RHS must be a character vector.")
   }
 
-  as_glue(paste0(e1, e2))
+  glue_data(
+    "{e1}{e2}",
+    .x = list(e1 = e1, e2 = e2),
+    .envir = parent.frame()
+  )
 }
 
 #' @importFrom methods setOldClass

tests/testthat/_snaps/glue.md

@@ -1,4 +1,4 @@
-# + method requires character vectors
+# `+` method requires character vectors
 
     Code
       as_glue("a") + 1
@@ -9,6 +9,13 @@
     Error <simpleError>
       LHS must be a character vector.
 
+# `+` method errors for inputs of incompatible size
+
+    Code
+      as_glue(letters[1:2]) + letters[1:3]
+    Error <simpleError>
+      Variables must be length 1 or 3
+
 # unterminated comment
 
     Code

tests/testthat/test-glue.R

@@ -507,22 +507,40 @@ test_that("throws informative error if interpolating a function", {
   }
 })
 
-test_that("+ method for glue works", {
-  expect_identical(glue("foo") + "bar", as_glue("foobar"))
-  expect_identical(glue("x = ") + "{x}", as_glue("x = {x}"))
+test_that("`+` method for glue works", {
+  expect_identical(glue("foo") + "bar", "foobar")
+  expect_identical("foo" + glue("bar"), "foobar")
+})
 
-  x <- c("a", "b", "c")
-  expect_identical("(" + as_glue(x) + ")", paste0("(", x, ")"))
+test_that("`+` method requires character vectors", {
+  expect_snapshot(error = TRUE, {
+    as_glue("a") + 1
+    1 + as_glue("a")
+  })
 })
 
 test_that("`+` method does not interpolate twice", {
-  expect_identical(glue("{x}", x = "{wut}") + "y", as_glue("{wut}y"))
+  expect_identical(glue("{x}", x = "{wut}") + "y", "{wut}y")
+})
+
+test_that("`+` method returns length-0 if there is a length-0 input", {
+  expect_identical(as_glue("hello") + character(), character())
 })
 
-test_that("+ method requires character vectors", {
+test_that("`+` method returns length-0 if there is a `NULL` input", {
+  expect_identical(as_glue("hello") + NULL, character())
+})
+
+test_that("`+` recycles", {
+  x <- c("a", "b", "c")
+  expect_identical("(" + as_glue(x) + ")", paste0("(", x, ")"))
+  y <- as.character(1:3)
+  expect_identical(as_glue(x) + y, c("a1", "b2", "c3"))
+})
+
+test_that("`+` method errors for inputs of incompatible size", {
   expect_snapshot(error = TRUE, {
-    as_glue("a") + 1
-    1 + as_glue("a")
+    as_glue(letters[1:2]) + letters[1:3]
   })
 })