From efefb586de1afd8b02128af9fd1e6e543eed655d Mon Sep 17 00:00:00 2001
From: edward-burn <9583964+edward-burn@users.noreply.github.com>
Date: Thu, 15 Aug 2024 21:56:45 +0100
Subject: [PATCH] updates after review

---
 NEWS.md                   |  4 ++--
 R/dup.R                   | 26 ++++++++++++++------------
 tests/testthat/test-dup.R | 13 ++++++-------
 3 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index 7898f266..9f575079 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -1,7 +1,7 @@
 # stringr (development version)
 
-* Add sep argument to `str_dup()` so that it is possible to repeat a string and 
-  add a separator between every repeated value. (@edward-burn, #564).
+* Add `sep` argument to `str_dup()` so that it is possible to repeat a string and 
+  add a separator between every repeated value (@edward-burn, #564).
 
 * In `str_replace_all()`, a `replacement` function now receives all values in
   a single vector. This radically improves performance at the cost of breaking
diff --git a/R/dup.R b/R/dup.R
index 9420d636..93a1d724 100644
--- a/R/dup.R
+++ b/R/dup.R
@@ -14,20 +14,22 @@
 #' str_dup(fruit, 1:3)
 #' str_c("ba", str_dup("na", 0:5))
 str_dup <- function(string, times, sep = NULL) {
-  vctrs::vec_size_common(string = string, times = times)
-  if(is.null(sep)){
+  size <- vctrs::vec_size_common(string = string, times = times)
+  if (is.null(sep)) {
     stri_dup(string, times)
   } else {
-  # stri_dup does not currently support sep
-  check_string(sep)
-  lapply(seq_along(string), function(i) {
-    if (length(times) == 1) {
-      paste(rep(string[i], times), collapse = sep)
-    } else {
-      paste(rep(string[i], times[i]), collapse = sep)
-    }
-  }) %>%
-    rlang::flatten_chr()
+    # stri_dup does not currently support sep
+    check_string(sep)
+    lapply(seq_along(string), function(i) {
+      if (size == 1) {
+        paste(rep(string[i], times), collapse = sep)
+      } else {
+        paste(rep(string[i], times[i]), collapse = sep)
+      }
+    }) %>%
+      rlang::flatten_chr()
   }
 
 }
+
+
diff --git a/tests/testthat/test-dup.R b/tests/testthat/test-dup.R
index 108a0307..d4b831c9 100644
--- a/tests/testthat/test-dup.R
+++ b/tests/testthat/test-dup.R
@@ -15,18 +15,17 @@ test_that("uses tidyverse recycling rules", {
 })
 
 test_that("uses sep argument", {
-  expect_equal(str_dup("a", 3, sep = ";"), "a;a;a")
-  expect_equal(str_dup("abc", 2, sep = "-"), "abc-abc")
-  expect_equal(str_dup("a", 3, sep = ";"), "a;a;a")
   expect_equal(str_dup("abc", 2, sep = "-"), "abc-abc")
   expect_equal(str_dup(c("a", "b"), 2, sep = "x"), c("axa", "bxb"))
   expect_equal(str_dup(c("aa", "bb", "ccc"), c(2, 3, 4), sep = ";"),
                c("aa;aa", "bb;bb;bb", "ccc;ccc;ccc;ccc"))
 
-  expect_error(str_dup("a", 3, sep = 1))
-  expect_error(str_dup("a", 3, sep = c("-", ";")))
-  expect_error(str_dup(c("aa", "bb"), c(2, 3, 4), sep = ";"))
-  expect_error(str_dup(c("aa", "bb", "cc"), c(2, 3), sep = ";"))
+  expect_identical(character(), str_dup(character(), 1, sep = ":"))
+
+  expect_snapshot(str_dup("a", 3, sep = 1), error = TRUE)
+  expect_snapshot(str_dup("a", 3, sep = c("-", ";")), error = TRUE)
+  expect_snapshot(str_dup(c("aa", "bb"), c(2, 3, 4), sep = ";"), error = TRUE)
+  expect_snapshot(str_dup(c("aa", "bb", "cc"), c(2, 3), sep = ";"), error = TRUE)
 
 })