Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provide wb_dims() #691

Merged
merged 4 commits into from
Jul 15, 2023
Merged

provide wb_dims() #691

merged 4 commits into from
Jul 15, 2023

Conversation

JanMarvin
Copy link
Owner

@JanMarvin JanMarvin commented Jul 14, 2023

@olivroy something I quickly typed down in addition to #681, thinking about ggplot2::aes() and how clean this approach is. This PR provides wb_dims() which can take either row/col, rows/cols or objects that can be coerced via as.data.frame().

  library(openxlsx2)
  
  # take data frames and col_names & row_names
  wb_dims(mtcars)
  wb_dims(mtcars, col_names = TRUE, row_names = TRUE)
  
  # take vectors
  wb_dims(letters)
  wb_dims(t(letters))
  
  # or time series
  wb_dims(AirPassengers)
  
  # single values
  wb_dims(1)
  
  # named 
  wb_dims(rows =  1:10, cols = 5:7)
  # wb_dims(rows =  1:10, row = 5:7)
  
  # single cell
  wb_dims(row = 5, col = 7)
  
  # two vectors  
  wb_dims(1:10, LETTERS)
  wb_dims(1:10, 1:26)
  
  # in a wb chain
  wb <- wb_workbook()$
    add_worksheet()$
    add_data(x = mtcars)$
    add_fill(
      dims = wb_dims(mtcars, col_names = TRUE),
      color = wb_color("yellow")
    )

This could be tweaked further, but might provide a simpler helper for users. A name to indicate that it is coming from openxlsx could be wb_dims().

Please let me know what you think!

[Edit:] Updated

@JanMarvin
Copy link
Owner Author

wb_dims() might be better, because dims is just an s away from dim and a small typo might quickly cause confusion.

@olivroy
Copy link
Collaborator

olivroy commented Jul 14, 2023

Oh yes, I like that, especially with the prefix!

With this, there wouldn't be a use-case for rowcol_to_dims() in the exported API?

could inform users of the new syntax.

But I think it's another step forward for a consistent, streamlined API

@JanMarvin
Copy link
Owner Author

Yes, this would be a more convenient and possibly simpler to remember function that creates the same output. It's a wrapper around functions.

@JanMarvin JanMarvin changed the title provide dims() provide wb_dims() Jul 14, 2023
R/utils.R Outdated Show resolved Hide resolved
@JanMarvin JanMarvin merged commit 77c9a83 into main Jul 15, 2023
9 checks passed
@JanMarvin JanMarvin deleted the dims branch July 15, 2023 17:32
@olivroy
Copy link
Collaborator

olivroy commented Jul 18, 2023

Hi @JanMarvin ,

after reviewing, wb_dims() may be enhanced

  • provide data_only option maybe?
  • For example in a vignette
wb_dims(mtcars)
"A1:K33"
# I propose:
wb_dims(mtcars, data_only = TRUE)
"A2:K33"
# it could be very useful when applying styling to the data.
# Edit2
wb_dims(mtcars, col_names = FALSE)
"A1:K32"
# you have to add
wb_dims(mtcars, col_names = FALSE, start_row = 2)
"A2:K33"
# to give the correct result. Which one do you think is correct?
# it would be good if this worked too
wb_dims(mtcars, cols = 4) 
# but you have to specify to make it work.
wb_dims( cols = 4, rows = seq_len(nrow(mtcars)) 
  • Error if only cols or rows is provided.
# now
wb_dims(cols = 4)
"A1"
# I suggest 
wb_dims(cols = 4)
"D1"

I can file a new issue if it's easier.

Edit:

I think that all arguments in ... should be named to avoid problems.

so this should fail

wb_dims(mtcars)

should be replaced by

wb_dims(x = mtcars) # or data = 

Also a personal wish (as I would love to think about the Excel result as little as possible.

Could something like this work?

wb_dims(x = mtcars, cols = "vs")

@JanMarvin
Copy link
Owner Author

wb_dims(x = mtcars, cols = "vs")

like wb_dims(mtcars, start_cols = "VS")?

But yes, I though about the same (regarding columns, rownames and data selection). Go for it if you want to improve it. I usually use the cell directly. Open a new issue or a pull request, I currently don't have time for this.

This was referenced Jul 18, 2023
@olivroy olivroy mentioned this pull request Aug 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants