-
Notifications
You must be signed in to change notification settings - Fork 11
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
Wb dims bug #1107
Wb dims bug #1107
Conversation
…ich is problematic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve added a few minor changes. Looks good otherwise!
expect_warning( | ||
expect_equal( | ||
wb_dims(x = names(mtcars), from_dims = "A2", above = 1), | ||
"A1:A11"), | ||
"rows cannot be above of row 1 " | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't understand this :(
I feel like if I read this literally. Let's use a simple example.
wb_dims(from_row = 3)
#> "A3"
# I feel like reading this literally, we just want the cell above.
wb_dims(from_row = 3, above = 1)
# I would expect this to be "A2"
Sorry for being slow here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The vector size is length(names(mtcars))
. Above 1 tries to add this vector on top of “A2“
. But there’s only place for a single value. Above, below, left and right simply don’t move integer rows in any direction, but in this case dim(x)
wise. I tried to make it easier to move around, without having to add the object size multiple times.
If the integer passed to above is greater than 1, the distance between from_dim and the new dim object will be 1 row. I can create a more detailed example later. I guess it’s easier to understand, if you have enough space to the left and right. Maybe start in "M40"
.
You are simply expecting a different behavior. I didn’t want to move the dims region row or column wise over the sheet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok ! thanks again! WIll merge now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, I hope you tell me if it makes no sense. I have added an example to discussion #1108 , hopefully this makes it clearer what I was intending. What I was striving for was minimizing position calculations. Because otherwise every time when the size of a data frame changes there was a chance of the calculation to be wrong.
Attempt to address partially #1104, by getting rid of some bugs?
This doesn't really work.
Previously, there was a large condition that would just evaluate the
below
above
,left
,right
code iffrom_dims
was supplied.This doesn't yet work, but I think it is a start.