Review wb_dims()

[olivroy]

Aims to fix #697 , follow up to #691

This PR aims to enable the following syntax

EDIT: Updated example to reflect latest API when using wb_dims() with x.

## add formatting to column names with the help of `wb_dims()`----
wb <- wb_workbook()
wb$add_worksheet("test wb_dims() with an object")
dims_mtcars_and_col_names <- wb_dims(x = mtcars)
wb$add_data(x = mtcars, dims = dims_mtcars_and_col_names)

# Put the font as Arial for the data
dims_mtcars_data <- wb_dims(x= mtcars, select = "data")
wb$add_font(dims = dims_mtcars_data, name = "Arial")

# style col names (with select = "col_names")
dims_column_names <- wb_dims(x = mtcars, select = "col_names")
wb$add_font(dims = dims_column_names, bold = TRUE, size = 13)

# add styling to column "cyl" (the 4th column) (only the data, the default)
# there are many options, but here is the preferred one
# if you know the column index, wb_dims(x = mtcars, cols = 4) also works.
dims_cyl <- wb_dims(x = mtcars, cols = "cyl")
wb$add_fill(dims = dims_cyl, color = wb_color("pink"))

# Add yellow to the vs col, and its column name
dims_vs_and_col_name <- wb_dims(x = mtcars, cols = "vs", select = "x")
wb$add_fill(dims = dims_vs_and_col_name , fill = wb_color("yellow"))

# Cond formatting to the mpg column.
# if specifying cols or rows, the default `select` is "data", 
# if not specifying rows or cols, the default behavior is "x"
wb$add_conditional_formatting(dims = wb_dims(x = mtcars, cols = "mpg"), type = "dataBar")

[JanMarvin]

Thanks for the pull request. I'll review it later this week. Its already past midnight and it's a bit to complicated for my sleepy head.

[JanMarvin]

I like the general direction of the pull request and have made a few comments. We might want to think about start_ arguments and what they are supposed to do.

[olivroy]

as mentioned in one of the commentd, I see them as from_row/col and they are implemented this way

[olivroy]

Here are the parts I think are more important. Sorry about the hard to read code in some places..

Maybe another wrapper be even clearer?

I had issues dealing with add_data(col_names = FALSE), so I warn if x has column names, but specify col_names = FALSE

Edit: thanks a lot for the review!

Edit 2: Feel free to close too. It`s more complicated than I wished it to be. I like the result, and think it can streamline styling and formatting, but I may be wrong ;)

Edit 3: maybe ask for another review to see if they think this can be useful?

[JanMarvin]

Edit: thanks a lot for the review!

Edit 2: Feel free to close too. It`s more complicated than I wished it to be. I like the result, and think it can streamline styling and >formatting, but I may be wrong ;)

Edit 3: maybe ask for another review to see if they think this can be useful?

Thanks for the PR and thanks for being open minded! I'll try to review the PR tomorrow or Sunday. I have spent way to much time these last two weeks on openxlsx2. Regarding other reviewers, I think this task is up to us :) That's usually the case with software development. Even R core has the issue that nobody reviews/tests draft versions, but everyone complains about "bugged" releases.

Regarding Edit 2 and the increased complexity: after all, I might have stated this already elsewhere, openxlsx2 provides a base and people can always write custom functions around the base. Therefore a more detailed wb_dims() function could be added in a tools or extra package if we decide against this. This way openxlsx2 does not have to provide everything for everyone.

[JanMarvin]

Nice example. Still, I don't like changing col_names from our default value for wb_dims(). You may drop it if x and cols are selected or can add some additional selection function. Something like "top, left, right, bottom, data" if you want to pick something explicitly. But picking the data part without the columns per default is somewhat counter intuitive.

[olivroy]

I have thought of a new and simpler implementation. I should push tomorrow.

Instead of relying on rows = 0 , I added an argument

select = "col_names"
I was able to simplify it a lot. may open a new PR as this one is getting cluttered

[olivroy]

HI @JanMarvin, this is ready for review!
I have simplified a lot the logic, added 2 helpers to concentrate most checks in a single place.
changed the default back
Updated the vignettes to showcase the latest API.
Added a select argument:

• "x" select data + col names and row names (default if rows or cols is not provided
• "data" selects data only (default if cols or rows is provided)
• "col_names" select col names only (I am not yet supporting select a single column name)
• "row_names" selects row names only (I am not yet supporting selecting a single row name)

this approach gets rid of the rows = 0 syntax! (I wrote it, but I found it confusing, so I can't imagine someone who didn't write it)

Ideally, it would be great to start a new PR with a clean state, but I don't know how to do that...)

EDIT: thanks a lot for your patience. It was a lot of tries, but I think it feels more right now, after the iterations!
Edit 2: I have put the updated example on the first comment of the PR #702 (comment)

[JanMarvin]

Thanks, my only regret is that this wb_dims() function is growing bigger and bigger. I didn't want to compete with cellranger.

Minor comments, otherwise if should be fine.

[JanMarvin]

Thanks. Let me know what you think about the NULL thing. Other than that we're probably fine. I can review it one last time, but it looks fine. 👍

[olivroy]

I put it as error, can always change behavior should need arise! but imo, it's better to be explicit!

I think that wb_dims() could make code easier to read, but using it withoutarguments is just mysterious. no need for that ;) it is easy to type wb_dims(1, 1)

[olivroy]

remove outdated example

[JanMarvin]

I think that wb_dims() could make code easier to read, but using it without arguments is just mysterious. no need for that ;) it is easy to type wb_dims(1, 1)
I didn't think of that, but yes wb_dims() is a helper just like wb_color(). If A1 is requested, there are a few possibilities.

[JanMarvin]

I'll squash merge once the CI is green. Thank you for sticking to it and for the PR!

Hope you had at least a tiny bit of fun and maybe learned a bit or two about openxlsx2 😎

[olivroy]

Tjanks for letting me!
More work than I imagined. thanks for your thorough revire.

Hopefully, it can help users!

It will help me ar least