JanMarvin / openxlsx2

openxlsx2 - read, write and modify xlsx files
https://janmarvin.github.io/openxlsx2/
Other
117 stars 9 forks source link

`wb_dims()` with `above` seems incorrect? #1104

Closed olivroy closed 3 weeks ago

olivroy commented 1 month ago

Just got to try above, below arguments. Sorry for being slow.

I think there may be an issue with above, unless I am misunderstanding. Will try to update the docs if I am mistaken..

wb_dims(from_dims = "A3:I3", above = 1)
#> "A2"

I would expect this to be "A2:I2"?

wb_dims(x = mtcars, from_dims = "A3", above = 1)
#> Warning:
#>  rows cannot be above of row 1 (integer position 1). resetting
#> "A1:K33"

I would expect this to be "A2:K34"

wb_dims(x = mtcars, from_dims = "A3", select = "col_names", above = 1)
#>   rows cannot be above of row 1 (integer position 1). resetting
"A1:K1"

I would expect this to be "A2:K2". Let me know what you think?

https://github.com/JanMarvin/openxlsx2/pull/960

JanMarvin commented 1 month ago

Thanks for looking into this!

Example 1

wb_dims(from_dims = "A3:I3", above = 1)
#> "A2"

I would expect this to be "A2:I2"?

I guess it is fine. We do not know the size of the element that is supposed to be placed there. If we use this cell "A2", to write data, this would work, because we currently are not checking, if the object fits into dims and simply increase dims until it fits.

We could pass an object, to get the wanted size, but that's another call:

# we have to use the t() trick, because otherwise it is assumed that vectors require vertical space
wb_dims(from_dims = "A3:I3", above = 1,  x = t(1:10), col_names = FALSE)
#> "A2:I2"

Just to confirm: the other arguments provide the same single cell in this case:

> wb_dims(from_dims = "A3:I3", below = 1)
#> "A4"
> wb_dims(from_dims = "A3:I3", right = 1)
#> "J3"
> wb_dims(from_dims = "A3:I3", left = 1) # with a warning
#> "A3"

Example 2

wb_dims(x = mtcars, from_dims = "A3", above = 1)
#> Warning:
#>  rows cannot be above of row 1 (integer position 1). resetting
#> "A1:K33"

I would expect this to be "A2:K34"

IIRC with the positioning parameters I intended to move the entire object around. If one wants to place mtcars directly above of cell "A3", the result would be something like "A-30:K1" and we reduce it to the valid worksheet region. Therefore the confusion might stem from the fact that there is not enough place above "A3". The arguments above, below, left, right in addition to an x object try to place the entire object n cells next to the from_dims location. The integers passed to above simply provide an indication to how much space should be between the origin and the new placement.

Example 3

wb_dims(x = mtcars, from_dims = "A3", select = "col_names", above = 1)
#>   rows cannot be above of row 1 (integer position 1). resetting
"A1:K1"

I would expect this to be "A2:K2". Let me know what you think?

That's a similar issue. As far as the implementation works, it's all right. It tries to get the range "A-30:K-30", because it tries to get space for the fullx` object, but selects only the column name of this object. Contrary to this:

> nams <- mtcars[NULL, , drop = FALSE]
> wb_dims(x = nams, from_dims = "A3", above = 1)
[1] "A2:K2"

Summary

Therefore I'd say everything works as intended, but obviously this does not mean that this is a good idea. I assume that these arguments have not been picked up a lot, therefore we could obviously try to modify them and change their behavior. My intention at the time was, that I had a work case, where I had to place a data frame onto a sheet and had to add several other same sized data frames around this original frame (something like mtcars and prop.table(mtcars)). And I needed a row with a formula below.

PS: this looks like a bug:

wb_dims(x = names(mtcars), from_dims = "A3", above = 1)
#> Error in if (frow < 1) { : argument is of length zero
olivroy commented 1 month ago

Thanks for the explanation! that's useful. I will review again once we get rid of bugs we both agree on!

Before fixing:

wb_dims(x = names(mtcars), from_dims = "A3", above = 1)
#> Error in if (frow < 1) { : argument is of length zero

There may be more bugs?

wb_dims(x = names(mtcars), from_row = 2)
#> " "A2:A12"
wb_dims(x = names(mtcars), from_row = 2, above = 1)
#> " "A2:A12"

Shouldn't that be "A1:A11"? (or "A1"?)

I will have to review again after.

Let's get to a place where there are no bugs and then we can discuss if some things make sens or not?

JanMarvin commented 1 month ago

Indeed wb_dims(x = names(mtcars), from_row = 2, above = 1) looks buggy too. Probably a strange interaction we didn't think about.

Let's get to a place where there are no bugs and then we can discuss if some things make sens or not?

Sounds good!

JanMarvin commented 3 weeks ago

Do you plan to revisit the documentation for wb_dims() anytime soon, @olivroy? Or shall we close this issue for now?

olivroy commented 3 weeks ago

Hi! That will probably happen next time I have to do something tricky with Excel! We can close for now!