Update vignettes to snake_case, and other code quality updates

[olivroy]

After converting arguments to snake case in #678, this PR updates some of the documentation, along with other code quality improvements.

Addresses #681, #643, #38, follow-up to #678

You may be best suited to review the reference index in _pkgdown.yml. before this PR, functions are listed twice. You made a great job of removing exports or adding @keywords internal in some places to reduce the number of exported functions to simplify the function index.

This PR may be a WIP, but may need your views on some topics.

Still not done with reviewing vignettes, not sure how thorough we want to be with row, cols -> dims.

The examples are not done yet.
I didn't have time to go through all vignettes.

When reviewing, please take a look at the rendered .Rd files to make sure nothing got messed up when I removed tags

• Moving test-related files out of R/ in tests/testthat/helper.R See https://r-pkgs.org/testing-design.html#sec-testing-design-tension
• Revisit vignettes (rename intro, so it appears as Get Started on the pkgdown website.
• Use dims
• fix typos
• Tweak slightly some vignettes
• Revisit function index
• Separate package options in its own .Rd file.
• Fix DESCRIPTION
• Remove @keywords internal for internal un-exported functions.

For ref, I didn't encounter breakages or R 4.2.3, on Windows!

Edit 2: about @keywords internal vs @noRd

the @keywords internal I removed were related to unexported functions that already hada @noRd tag

Basically,

@keywords internal: For exported functions where you don't want the function to show up in the reference index, but still want accessible for thr help ?function (i.e. still create the Rd file)

@noRd Usually For non-exported functions with roxygen documentation, this ensures that no .Rd file is created. therefore @keywords internal is redundant.

I did this cleanup to better represent which functions are exported, but don't show up in the ref index (i.e are not part of the suggested API, or deprecated) but still exported to avoid code breakage.

[olivroy]

I reverted the changes in R/write.R along with the documentation! and opened #683

[JanMarvin]

Sorry about that. I am a beginner in managing PRs, and got carried away.

I will answer your questions if you have any, but basically, this doesn't change anything. I didn't touch the logic anywhere, nor functions.

The only weird diff I got was when re-rendering the README (I didn't push it, as it was unrelated to what I did, but will do if you think it is fine. (for ref, last time you touched readme was in #632

Oh this should be fine. I probably didn't recreate it after the last modifications. Thanks letting me know

[JanMarvin]

I left some comments and questions. I reviewed the changes (aside from the huge write.R ones) and I'm generally fine. Showstoppers remain:

• the write changes are still in this pull request No they are not! My mistake.
• not sure about the wrapper to testthat change. Let's give Jordan a moment - maybe until tomorrow - to comment. After all we can always move the file back to the R folder

[olivroy]

Hi,

Thanks for the review. I will go through it one last time,
and I did revert the R/write.R change in 45eb52e

ok, let's wait till tomorrow to merge.

You may want to double check the vignette with the most changes just to make sure!

[olivroy]

Here are the last places I wanted to highlight

[JanMarvin]

Good to go from my view, thanks a lot! As said, lets give Jordan a few hours to veto the movement of the expect_wrapper() function. I get that you are eager to have it merged, but we are already in lightspeed mode here.

[JanMarvin]

Thanks again for the PR, merged!

[olivroy]

Thanks for reviewing this fast!

I think the website looks great: https://janmarvin.github.io/openxlsx2/dev/

I will try switching from openxlsx to openxlsx2 in my personal scripts with the latest dev and will let you know the points of friction!

[JanMarvin]

Once a pull request gets this large it is blocking everything else. It would have become a pita to update this or other pull requests. Usually it is a better strategy to keep things a tiny bit smaller. But I do not expect any fallout from it and it was way simpler to review than #683 , this one will have to wait a few days. Maybe I can review it over the weekend

[jmbarbone]

Good to go from my view, thanks a lot! As said, lets give Jordan a few hours to veto the movement of the expect_wrapper() function. I get that you are eager to have it merged, but we are already in lightspeed mode here.

Sorry, I don't think I ever saw this (usually good about checking my mentions) and just happened to notice this while looking at the wrapper datetime failures.

From my understanding and use of the tests/testthat/helper.R, this may not be the ideal place for our wrapper functions. While the custom expectations are called in testing, moving them from the R directory makes them inaccessible in installed packages; i.e., I can't openxlsx2:::expect_wrappers() or source a system.file() (because the test files are not packaged) (see #686 (comment)).

Use of the R directory is recommended in 14.3.1 Hiding in plain sight: files below R/. The complexity of expect_wrapper() is better handled in R/ where other functions in the helper.R can be appropriately kept there. If we flesh out expect_equal_workbooks(), that could even be an export for users wishing to perform their own tests.

[JanMarvin]

Oh, sorry I think I mentioned you somewhere in this PR and wasn't sure if you still cared and didn't want to bother you with repeated mentions. Sorry, if you missed it.

Regarding the function, I'll leave this for you guys to decide. I'm fine with either. I was hoping that we one day have a expect_equal() for workbooks, but I'm not seeing this anytime soon and it's not the highest priority right now, because we're currently pretty stable.