Be strict about cols, widths, hidden lengths. (#991)

* Be strict about `cols`, `widths`, `hidden` lengths. * address comments + throw warnings for now instead of errors
JanMarvin · Apr 30, 2024 · 45e3c32 · 45e3c32
1 parent 0cc3017
commit 45e3c32
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 9 deletions.
diff --git a/NEWS.md b/NEWS.md
@@ -25,6 +25,8 @@ wb_dims(x = mtcars, cols = "non-existent-col")
 # Now errors
 ```
 
+* `wb_set_col_widths()` is more strict about its arguments. If you provide `cols`, `widths`, or `hidden` don't have appropriate length, it will throw a warning. This may change to an error in the future, so it is recommended to use appropriate values.
+
 
 ***************************************************************************
 

diff --git a/R/class-workbook.R b/R/class-workbook.R
@@ -3797,9 +3797,26 @@ wbWorkbook <- R6::R6Class(
         stop("hidden argument is longer than cols.")
       }
 
+      compatible_length <- length(cols) %% length(widths) == 0
+
+      if (!compatible_length) {
+        # needed because rep(c(1, 2 ), length.out = 3) is successful,
+        # but not clear if this is what the user wanted.
+        warning("`cols` and `widths` should have compatible lengths.\n",
+             "`cols` has length ", length(cols), " while ",
+             "`widths` has length ", length(widths), ".")
+      }
+
       if (length(widths) < length(cols)) {
         widths <- rep(widths, length.out = length(cols))
       }
+      compatible_length <- length(cols) %% length(hidden) == 0
+
+      if (!compatible_length) {
+        warning("`cols` and `hidden` should have compatible lengths.\n",
+             "`cols` has length ", length(cols), " while ",
+             "`hidden` has length ", length(hidden), ".")
+      }
 
       if (length(hidden) < length(cols)) {
         hidden <- rep(hidden, length.out = length(cols))

diff --git a/tests/testthat/test-class-workbook.R b/tests/testthat/test-class-workbook.R
@@ -6,7 +6,7 @@ test_that("Workbook class", {
 })
 
 
-test_that("wb_set_col_widths", {
+test_that("wb_set_col_widths works", {
 # TODO use wb$wb_set_col_widths()
 
   wb <- wbWorkbook$new()
@@ -39,17 +39,10 @@ test_that("wb_set_col_widths", {
     wb$worksheets[[1]]$cols_attr
   )
 
-  # a few more errors
-  expect_error(wb$set_col_widths("test", cols = "Y", widths = 1:2))
-  expect_error(wb$set_col_widths("test", cols = "Y", hidden = 1:2))
-
-
-
-
   wb <- wb_workbook()$
     add_worksheet()$
     set_col_widths(cols = 1:10, widths = (8:17) + .5)$
-    add_data(x = rbind(8:17), colNames = FALSE)
+    add_data(x = rbind(8:17), col_names = FALSE)
 
   exp <- c(
     "<col min=\"1\" max=\"1\" bestFit=\"1\" customWidth=\"1\" hidden=\"false\" width=\"9.211\"/>",
@@ -76,6 +69,16 @@ test_that("wb_set_col_widths", {
 
 })
 
+test_that("set_col_widths informs when inconsistent lengths are supplied", {
+  wb <- wbWorkbook$new()
+  wb$add_worksheet("test")
+
+  expect_warning(wb$set_col_widths(cols = c(1, 2, 3), widths = c(2, 3)), "compatible length")
+  expect_error(wb$set_col_widths(cols = "Y", widths = 1:2), "More widths than column")
+  expect_error(wb$set_col_widths("test", cols = "Y", hidden = 1:2), "hidden argument is longer")
+  expect_warning(wb$set_col_widths(cols = c("X", "Y", "Z"), hidden = c(1, 0)), "compatible length")
+})
+
 test_that("option maxWidth works", {
 
   op <- options("openxlsx2.maxWidth" = 6)