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

wb_add_conditional_formatting dims argument broken #654

Closed
Michiel91 opened this issue Jun 21, 2023 · 6 comments
Closed

wb_add_conditional_formatting dims argument broken #654

Michiel91 opened this issue Jun 21, 2023 · 6 comments

Comments

@Michiel91
Copy link

When trying the wb_add_conditional_formatting function I noticed that the dims argument is broken. When supplying any (valid) dimension, the following error is returned:
Error in if (!all(vapply(cellCoords, is_integer_ish, NA))) { : missing value where TRUE/FALSE needed In addition: Warning messages: 1: In min(rows) : no non-missing arguments to min; returning Inf 2: In min(cols) : no non-missing arguments to min; returning Inf

Checking the function code, I see that the dims argument is not passed to the underlying add_conditional_formatting function.

The rows and cols arguments work, but they trigger a warning when supplying a previously generated index range. For example when creating a variable called my_cols <- 1:6 and supplying this to the cols argument, the following is shown:
Warning message: In wb$clone()$add_conditional_formatting(sheet = sheet, cols = cols, : cols > 2, will create range from min to max.

Thanks for looking into this.

@JanMarvin
Copy link
Owner

JanMarvin commented Jun 21, 2023

@Michiel91 please provide a minimal reproduceable example. Dims for conditional formatting were introduced just recently.

[Edit] fixed autocorrect

@JanMarvin
Copy link
Owner

This should work: #645

@JanMarvin JanMarvin added the question ❓ Further information is requested label Jun 21, 2023
@Michiel91
Copy link
Author

Michiel91 commented Jun 21, 2023

Hi @JanMarvin,

As pointed out it should be obvious from the code of wb_add_conditional_formatting that the dims argument is not used anywhere in the function (not passed to the underlying add_conditional_formatting function). Nevertheless hereby a minimum reproduceable example to mimic the error and warning I described:

data <- matrix(sample(0:10, 5), 5, 4)

wb <- openxlsx2::wb_workbook() %>%
	openxlsx2::wb_add_worksheet("test") %>%
	openxlsx2::wb_add_data(x = data, 
												 sheet = "test")

# Triggers error: Apply conditional formatting using dims
wb %<>%
	openxlsx2::wb_add_conditional_formatting(sheet = "test",
																					 dims = "A1:B5",
																					 type = "between",
																					 rule = c(2, 4))
# Triggers warning: Apply conditional formatting using predefined row and col range
row_range <- 1:5
col_range <- 1:4

wb %<>%
	openxlsx2::wb_add_conditional_formatting(sheet = "test",
																					 rows = row_range,
																					 cols = col_range,
																					 type = "between",
																					 rule = c(2, 4))

@JanMarvin JanMarvin removed the question ❓ Further information is requested label Jun 21, 2023
@JanMarvin
Copy link
Owner

Thanks, for the example, I created a pull request to fix this. It's quite warm in my attic apartment and this saved me searching what exactly is the problem in that case. You were completely right and this should be fixed right away.

@JanMarvin
Copy link
Owner

The fix is now in main

@Michiel91
Copy link
Author

Thanks for the fix @JanMarvin!

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

No branches or pull requests

2 participants