check for illegal sheetnames

[ycphs]

fix for #211

[jonas-hag]

Thanks for the quick PR! Unfortunately it doesn't solve all problems:

backslash

\ is an escape string in R, so things get complicated. You can't have a single \ in the name, e.g. sheetName = "test\" doesn't work because then the " gets escaped and your R code is messed up. However, something like sheetName = "test\test" works because then \t is interpreted as a tab and the resulting sheet name is "test est". It seems that there is no way of catching this (I think the new string literals would allow this, see https://mpopov.com/blog/2020/05/22/strings-in-r-4.x/, but then you need to depend on R >= 4.0.0). But people could try to escape the backslash in the sheet name to actual include one in the name like so: sheetName = test\\test. This is not caught by your regex currently and leads to an Excel error.

I think this fixes it: grepl(pattern = "([/\\\\\\*'?\\[:\\]+])", perl = TRUE, x = sheetName)

Currently, the backslash is not shown in the error message, this fixes it: stop("Illegal character in sheet names. Don't use the following [ ] * / \\\ ? :")

empty name

An empty name is not caught currently, this fixes it: if (grepl(pattern = "([/\\\\\\*'?\\[:\\]+])", perl = TRUE, x = sheetName) || nchar(sheetName) == 0)
One should probably also update the error message.

location of the check

I find it a bit confusing that these name checks are done in the WorkbookClass while the name length check is done in the wrapper functions like addWorksheet. Maybe one could unify this in one place?

minor detail:
probably better style would be perl = TRUE instead of perl = T

[ycphs]

Thank you for Your feedback.

I took over the maintenance of the package some years ago. So I didn't set up the structure.

• I totally agree that the location of functions is not consistent over the entire package
• Please feel free to contribute and/or clean up the structure

[jonas-hag]

Thanks for the update! It solves the problem, now also empty names and \\ lead to an error :)

other observations:

• so far only implemented in addWorksheet and not in the other functions
• possibly good to update the error message and mention the forbidden empty name
• missing tests for \\ and empty string

Thanks for the explanation! I'll look into the structure and see if I can clean it a bit

[jonas-hag]

I try to make my own PR to address the issues I've mentioned in my last comment and get a bit better structure. I need some input on where to place the check function.

• so far, one check function validateSheet is implemented as a method of the Workbook class which checks if a sheet exists in a workbook
• another check function validateColour is implemented as its own function in the helperFunctions.R file and called in the wrapper functions like addWorksheet
• currently, a little bit of name checking is done directly in R6 methods, e.g. for addWorksheet or cloneWorksheet

I don't have much experience working with R6 classes, so I'm not sure what is better, however I think the checking should be performed either only in the wrapper or in the R6 classes.

As currently a lot of checking is done within the wrapper functions I would suggest to write a helper function to validate the sheet name and call this in the wrapper functions, @ycphs what do you think?

[jonas-hag]

Also, I suggest to rename validateSheet to validateSheetExists to make clear that this function checks if a sheet exists in the workbook object and doesn't check if a sheet name is valid.

[JanMarvin]

This looks like it was fixed, but never merged and that the discussion was wandering of to unrelated topics. Can this be merged?

[JanMarvin]

I was bitten by this today and used this workaround:

sheets <- wb$sheet_names

for (sheet in sheets) {

  sheet_name <- sheet
  fixed_name <- openxlsx:::replaceIllegalCharacters(sheet)

  if (sheet_name != fixed_name)
   renameWorksheet(wb, sheet = sheet_name, newName = fixed_name)

}

[github-actions]

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.