CHOP-CGTInformatics / REDCapTidieR

Makes it easy to read REDCap Projects into R
https://chop-cgtinformatics.github.io/REDCapTidieR/
Other
33 stars 8 forks source link

Update openxlsx2 arguments #161

Closed olivroy closed 1 year ago

olivroy commented 1 year ago

Description

Use openxlsx2 new arguments (after v0.8). Everything was moved to snakecase.

See https://janmarvin.github.io/openxlsx2/news/index.html#openxlsx2-08 or https://janmarvin.github.io/openxlsx2/reference/openxlsx2-deprecated.html.

Since openxlsx2 has reached v1.0, this kind of breaking changes should be minimal in the future.

Also, since this is a CRAN package, renv should not be used, but I didn't commit these changes.

Proposed Changes

List changes below in bullet format:

Issue Addressed

Fix #160

PR Checklist

Code Review

This section to be used by the reviewer and developers during Code Review after PR submission

Code Review Checklist


rsh52 commented 1 year ago

@olivroy Thank you for the help and PR submission, the proposed changes look sound to me and appreciate the comments you added related to other updates that may null some manual handling we introduced.

Not using renv on CRAN packages is news to us, do you have anything to refer us to that supports this? We've learned to rely on it for a lot of our collaborative work for environment preservation.

The checks for this are likely failing due to the forked branch's missing access to our GitHub actions secrets. I think the best plan of action here is to merge this PR, confirm the checks pass on main and then follow up with an internal PR that handles the additional suggestions.

olivroy commented 1 year ago

Not using renv on CRAN packages is news to us, do you have anything to refer us to that supports this? We've learned to rely on it for a lot of our collaborative work for environment preservation.

@rsh52 It is just that CRAN runs checks on the packages latest versions, usually. Using renv possibly prevents you from catching these on CI in GitHub.

Notes are in renv docs. https://rstudio.github.io/renv/articles/packages.html

But I do not have strong views, it was just a little bit harder to create this PR ;)

Thanks for merging!

Edit: my comment is specifically related to CRAN package, I think that versioning projects is a really good asset otherwise

rsh52 commented 1 year ago

@olivroy Makes sense, if we find it becomes terribly burdensome or results in missed issues then we'll consider deactivating it. For now it's helped a lot with other things : )

As expected the checks are passing on main, we'll work on the other improvements soon. Thanks a lot for the help!