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

[dims] simplify wb_dims(). fixes #1124 #1125

Merged
merged 2 commits into from
Sep 2, 2024
Merged

[dims] simplify wb_dims(). fixes #1124 #1125

merged 2 commits into from
Sep 2, 2024

Conversation

JanMarvin
Copy link
Owner

This breaks a few previously valid cases, where we were working around broken input. The basic idea is that we have a single dim that can be reduced with selections of columns or rows and moved around in positive ranges.

@JanMarvin
Copy link
Owner Author

I have tried to clean up the existing code. All these nested conditions have given me one too many headaches. Since CRAN has not yet contacted me about the build errors in the vignette, I will postpone the next release for a while. At the moment, however, I suspect that this is a problem with the vignette or the build server. So maybe it will just go away.

@JanMarvin JanMarvin force-pushed the gh_issue_1124 branch 4 times, most recently from c814e97 to 7d10b8d Compare August 31, 2024 15:31
@@ -701,6 +714,9 @@ wb_dims <- function(..., select = NULL) {
names(args) <- nams
n_unnamed_args <- length(which(!nzchar(nams)))
all_args_unnamed <- n_unnamed_args == len

rows_arg <- args[[rows_pos]]
cols_arg <- args[[cols_pos]]
}
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either have x or rows_arg/cols_arg.

tests/testthat/test-utils.R Show resolved Hide resolved
tests/testthat/test-utils.R Show resolved Hide resolved
@@ -206,11 +206,11 @@ test_that("`wb_dims()` can select content in a nice fashion with `x`", {

# Selecting a row range
dims_row1_to_5 <- "B3:L7"
expect_equal(wb_dims_cars(rows = 1:5), dims_row1_to_5)
expect_equal(wb_dims(x = mtcars, from_row = 2, from_col = "B", rows = 1:5), dims_row1_to_5)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like these functions that are defined on top of a testthat file and then it breaks at the bottom, how am I supposed to know what is going on? 😅

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah! While writing the tests, I was seeing if it actually worked. I should have converted the tests to standalone pieces at the end.

…, where we were working around broken input. The basic idea is that we have a single dim that can be reduced with selections of columns or rows and moved around in positive ranges.
Copy link
Collaborator

@olivroy olivroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the changes in tests. Sorry for creating a monster :(

@JanMarvin
Copy link
Owner Author

Rome was not built in a day, and I believe that we have already achieved a lot from humble beginnings. As you pointed out some time ago, I consider wb_dims() to be an essential part of openxlsx2.

Clearing out is always easier than creating something new! Thanks for reviewing :)

@JanMarvin JanMarvin merged commit 1c709b8 into main Sep 2, 2024
9 checks passed
@JanMarvin JanMarvin deleted the gh_issue_1124 branch September 2, 2024 18:17
@olivroy olivroy linked an issue Sep 2, 2024 that may be closed by this pull request
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.

[dims] unexpected result with non continuous rows and x
2 participants