Be strict about cols, widths, hidden lengths.

[olivroy]

May want to change errors for warnings?

two things here

1. hidden is documented as a vector of logical, but class is not enforced, and in tests, integers are provided?
2. I tripped on the length difference of cols and widths

At some point, my script was like that

wb <- wb_workbook()
wb$add_worksheet()
wb$set_col_widths(widths = c(10, 15), cols = c(2, 3))

but then I realized that I needed a wider column 4 and I did this:

wb <- wb_workbook()
wb$add_worksheet()
wb$set_col_widths(widths = c(10, 15), cols = c(2, 3, 4))

or the opposite, but to realize very much later that it was the wrong thing.

Base R recycling rules are sometimes not what you expect

rep(c(1, 2 ), length.out = 3) # is successful,
#> 1, 2, 1

To avoid breaking changes, I could change this to warnings. I used %% to only error if one is not a multiple of the other,

[JanMarvin]

I agree that seems a bit sketchy. I prefer non breaking changes very much. Preferably they don’t interfere with me as user at all. If I was happy with the code a few months ago, why would I change my mind?

I will think about it over the weekend. Thanks for the PR and the explanations.

[olivroy]

No worries, this lacks polishing, but wanted to bake a PR before I forgot about this and / or get bitten again.

[JanMarvin]

We might have to adjust the changes to test order. It seems that at least one test is now flaking on CI

[olivroy]

We might have to adjust the changes to test order. It seems that at least one test is now flaking on CI

Does adding testsetup() on top of test-readSources or test-cloneWorksheet work?

[JanMarvin]

Not sure, it’s just one test is starting to fail because a testfile is not yet available

[olivroy]

Maybe it is just gh API limitations then..

[JanMarvin]

Hi @olivroy , sorry, me opening #1005 was a little late and I didn't want to rush you. I usually strive to announce new releases a bit earlier so that people (well, mostly me) can make time to finish PRs and do further cleanups.

Unfortunately this time I have received a CRAN notification to push a fix for the isBlankString() usage and I want to have a little buffer time in case there are other hickups with the submission. (May 1st is labor day in Germany and a national holiday, that's why I picked it.) Just let me know how far this PR is and if it needs additional development time so that we can make time for this.

[olivroy]

to me, it feels like it is finished. just adding warnings in the case of incompatible widths / hidden supplied to set_col_widths.

Leaving the rest unchanged and moving tests in a new entry to catch wb_set_col_widths

It feels less intrusive than throwing error, but signaling that the input may provide unexpected results

[JanMarvin]

Thanks, I just now realize that you have simply moved the expect_error() tests, but I actually feel that testing against some condition is worth it (we once had a expect_warning() test that succeeded, but the warning was produced for reasons unrelated to openxlsx2 and not what was supposed to be covered 😀

Otherwise good to go!

[olivroy]

Thanks! So, I guess you can merge and then test revdeps?

[JanMarvin]

Please fix the lintr warning and the probably unintended change in the example code. Feel free to squash merge afterwards. Thanks!

[olivroy]

Fixed both the lintr warning and the typo.. Sometimes RStudio types things for me :(