validate_new_sheet now tries to fix the sheet name instead of errors. c…

…loses #687 (#705)
JanMarvin · Jul 23, 2023 · 91b4915 · 91b4915
1 parent ae0a6ba
commit 91b4915
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 19 deletions.
diff --git a/R/class-workbook.R b/R/class-workbook.R
@@ -406,8 +406,8 @@ wbWorkbook <- R6::R6Class(
       }
 
       sheet <- as.character(sheet)
+      private$validate_new_sheet(sheet)
       sheet_name <- replace_legal_chars(sheet)
-      private$validate_new_sheet(sheet_name)
 
 
       newSheetIndex <- length(self$worksheets) + 1L
@@ -571,8 +571,8 @@ wbWorkbook <- R6::R6Class(
       }
 
       sheet <- as.character(sheet)
+      private$validate_new_sheet(sheet)
       sheet_name <- replace_legal_chars(sheet)
-      private$validate_new_sheet(sheet_name)
 
       if (!is.logical(grid_lines) | length(grid_lines) > 1) {
         fail <- TRUE
@@ -761,7 +761,10 @@ wbWorkbook <- R6::R6Class(
     #' @param old name of worksheet to clone
     #' @param new name of new worksheet to add
     clone_worksheet = function(old = current_sheet(), new = next_sheet()) {
-      private$validate_new_sheet(new)
+
+      sheet <- new
+      private$validate_new_sheet(sheet)
+      new <- sheet
       old <- private$get_sheet_index(old)
 
       newSheetIndex <- length(self$worksheets) + 1L
@@ -2708,7 +2711,9 @@ wbWorkbook <- R6::R6Class(
 
       # should be able to pull this out into a single private function
       for (i in seq_along(pos)) {
-        private$validate_new_sheet(new_name[i])
+        sheet <- new_name[i]
+        private$validate_new_sheet(sheet)
+        new_name[i] <- sheet
         private$set_single_sheet_name(pos[i], new_name[i], new_raw[i])
         # TODO move this work into private$set_single_sheet_name()
 
@@ -7060,20 +7065,33 @@ wbWorkbook <- R6::R6Class(
 
       sheet <- as.character(sheet)
       if (has_illegal_chars(sheet)) {
-        stop("illegal characters found in sheet. Please remove. See ?openxlsx2::clean_worksheet_name")
+        warning("Fixing: removing illegal characters found in sheet name. See ?openxlsx2::clean_worksheet_name.")
+        sheet <- replace_illegal_chars(sheet)
       }
 
       if (!nzchar(sheet)) {
-        stop("sheet name must contain at least 1 character")
+        warning("Fixing: sheet name must contain at least 1 character.")
+        sheet <- paste("Sheet", length(self$sheet_names) + 1)
       }
 
       if (nchar(sheet) > 31) {
-        stop("sheet names must be <= 31 chars")
+        warning("Fixing: shortening sheet name to 31 characters.")
+        sheet <- stringi::stri_sub(sheet, 1, 31)
+        if (any(duplicated(c(sheet, self$sheet_names))))
+          stop(
+            "Cannot shorten sheet name to a unique string. ",
+            "Please provide a unique sheetname with maximum 31 characters."
+          )
       }
 
       if (tolower(sheet) %in% self$sheet_names) {
-        stop("a sheet with name '", sheet, '"already exists"')
+        warning("Fixing: a sheet with name '", sheet, '"already exists. Creating a unique sheetname"')
+        ## We simply append (1), while spreadsheet software would increase
+        ## the integer as: Sheet, Sheet (1), Sheet (2) etc.
+        sheet <- paste(sheet, "(1)")
       }
+
+      assign("sheet", sheet, parent.frame())
     },
 
     set_current_sheet = function(sheet_index) {

diff --git a/tests/testthat/test-Worksheet_naming.R b/tests/testthat/test-Worksheet_naming.R
@@ -16,13 +16,13 @@ test_that("legal characters are allowed", {
 
 test_that("illegal characters are not allowed", {
   wb <- wb_workbook()
-  expect_error(wb$add_worksheet("forward\\slash"), "illegal")
-  expect_error(wb$add_worksheet("back/slash"),     "illegal")
-  expect_error(wb$add_worksheet("question?mark"),  "illegal")
-  expect_error(wb$add_worksheet("asterik*"),       "illegal")
-  expect_error(wb$add_worksheet("colon:"),         "illegal")
-  expect_error(wb$add_worksheet("open[bracket"),   "illegal")
-  expect_error(wb$add_worksheet("closed]bracket"), "illegal")
+  expect_warning(wb$add_worksheet("forward\\slash"), "illegal")
+  expect_warning(wb$add_worksheet("back/slash"),     "illegal")
+  expect_warning(wb$add_worksheet("question?mark"),  "illegal")
+  expect_warning(wb$add_worksheet("asterik*"),       "illegal")
+  expect_warning(wb$add_worksheet("colon:"),         "illegal")
+  expect_warning(wb$add_worksheet("open[bracket"),   "illegal")
+  expect_warning(wb$add_worksheet("closed]bracket"), "illegal")
 })
 
 test_that("validate sheet", {
@@ -33,10 +33,10 @@ test_that("validate sheet", {
   expect_error(wb$add_worksheet(-1), "positive")
   expect_error(wb$add_worksheet(1), NA)
   expect_error(wb$add_worksheet(1), "index")
-  expect_error(wb$add_worksheet(""), "at least 1 character")
+  expect_warning(wb$add_worksheet(""), "at least 1 character")
   expect_error(wb$add_worksheet("0123456789012345789012345678901"), NA)
-  expect_error(wb$add_worksheet("01234567890123457890123456789012"), "31")
+  expect_error(expect_warning(wb$add_worksheet("01234567890123457890123456789012"), "31"), "Cannot shorten")
   expect_error(wb$add_worksheet("my_sheet"), NA)
-  expect_error(wb$add_worksheet("my_sheet"), "already exists")
+  expect_warning(wb$add_worksheet("my_sheet"), "already exists")
   expect_error(wb$add_worksheet(" "), NA)
 })
diff --git a/tests/testthat/test-names.R b/tests/testthat/test-names.R
@@ -22,6 +22,17 @@ test_that("names", {
   expect_error(wb$set_sheet_names(new = "S1"), "does not contain any sheets")
 
   wb$add_worksheet("S1")
-  expect_error(wb$set_sheet_names(new = paste(rep("a", 32), collapse = "")), "31 chars")
+  expect_warning(wb$set_sheet_names(new = paste(rep("a", 32), collapse = "")), "31 characters")
   file.remove(tmp)
+
+  wb <- wb_workbook()
+  expect_warning(wb$add_worksheet(paste(rep("a", 32), collapse = "")), "31 characters")
+  expect_error(
+    expect_warning(
+      wb$add_worksheet(paste(rep("a", 32), collapse = "")),
+      "31 characters"
+    ),
+    "unique string"
+  )
+
 })