Avoid character returns in wb functions

[JanMarvin]

Certain wb functions return character strings. Recently stumbled over this here:

library(openxlsx2)
wb <- wb_workbook() %>% 
  wb_add_worksheet() %>% 
  wb_save(path = temp_xlsx()) %>%
  wb_open()
#> Error: wb must be class wbWorkbook or R6

I expected wb to remain a workbook, but it had turned into a string with wb_save().

Another case where a wb function returns something other than a wbWorkbook is wb_get_cell_styles():

library(openxlsx2)
wb <- wb_workbook() %>% 
  wb_add_worksheet() %>% 
  wb_get_cell_style(dims = "A1") %>% 
  wb_open()
#> Error: wb must be class wbWorkbook or R6

I'm wondering, if we should avoid this. I'm thinking about something like a path and message field, where we store the path we read from and save to.

[jmbarbone]

I don't see anything wrong with the $get_*() methods as it should be obvious that the return value isn't going to be the wbWorkbook.

$save() maybe. I think I remember something specific about returning the file path, but I don't think that is very necessary.

[JanMarvin]

I've pushed a draft of my idea here: #379 (just something that compiles and returns something.

With this PR both chunks above return a workbook. They use the three fields:

• path: This was especially easy in wb_save() this functions simply behaved differently compared to wb$save(). Therefore I'd say it's simply a bug.
• message: We could use this to add calls over the lifespan of the workbook. I've added match.call() which is not entirely helpful, but wanted something quick. We could return this with wb$print() or something similar.
• returns: We can simply push wb_get results to return and use them in a second call. If we want to return something to the user, we can message the results.

This way wb functions could always return a workbook. IMHO this makes the wb function more predictable and chaining and piping simpler to use.

[jmbarbone]

Mentioning that that the messages field is likely not stable, though I do like the idea of some history tracking. Maybe log might be a better naming?

I'm not sure I quite understand the returns value. Does that mean wb$get_authors() is going to return the wbWorkbook object invisibly and not a character vector for authors? I don't think managing multiple return values is going to be very pleasant

The $get_*() methods should be clear enough that they don't return the wbWorkbook object but instead a field or some other sort of attribute. They wouldn't make sense piped:

wb <- 
  wb_workbook() |>
  wb_add_worksheet() |>
  wb_add_data() |>
  # what would this do? how do I get the authors?
  # if it is only saving the value to `results` then it's more of a
  # _store_ or _catch_.
  wb_get_authors() |>  
  wb_add_worksheet() |>
  wb_add_data()

# access directly?  no means of performing checks, validations:
wb$authors

# access with another function? feels redundant to have to get the value then get it again
wb |>
  wb_get_authors() |>
  wb_get_result("authors")

# current methods are essentially accessing directly but can contain 
# some additional checks and validations
wb$get_authors()
wb_get_authors(wb)

[JanMarvin]

sorry replaced your comment 🤦
wanted to quote on mobile ... Restored

This is what I had in mind: we could wrap it in functions not called wb_. Though until now it was just an idea and a quick draft.

wb |>
  wb_get_authors() |>
  wb_get_result()

Or

wb |> get_authors()

[jmbarbone]

I often accidentally delete my own comments!

To me, the wb_* prefix denotes that the function works on a wbWorkbook object, not that it returns that object. The $get_*(), $set_*(), and $add_*() methods are all wrapped by wb_get_*(), etc; a different naming convention would make these feel a little weird. In the documentation we can note that get functions will return a value/attribute of the workbook but all other functions will return the wbWorkbook object. That should be pretty consistent with other packages.

[JanMarvin]

For me this is about predictable code. If I am surprised by the output as author, what else can I expect from the user.

log is fine to me as well and I drafted this from an earlier suggestion by yourself for some change tracking 😉