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

Update openxlsx2 arguments #161

Merged
merged 2 commits into from
Sep 12, 2023
Merged

Update openxlsx2 arguments #161

merged 2 commits into from
Sep 12, 2023

Conversation

olivroy
Copy link
Contributor

@olivroy olivroy commented Sep 9, 2023

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:

  • Added 2 suggestions of new openxlsx2 functionalities for your future consideration
  • openxlsx2 provides the oportunity to ignore xlsx errors, so a workaround you are using could be removed
  • with validate_new_sheet now tries to fix the sheet name. closes #687 JanMarvin/openxlsx2#705, openxlsx2 does a more robust sheet name validation, your workaround could also be removed.
  • Used openxlsx2 new arguments
  • Indicated where arguments were provided but unused (i.e. sep.names)

Issue Addressed

Fix #160

PR Checklist

  • New/revised functions have associated tests
  • New/revised functions that update downstream outputs have associated static testing files (.RDS) updated under inst/testdata/create_test_data.R
  • New tests that make API calls use httptest::with_mock_api and any new mocks were added to tests/testthat/fixtures/create_httptest_mocks.R
  • New/revised functions use appropriate naming conventions
  • New/revised functions don't repeat code
  • Code changes are less than 250 lines total
  • Issues linked to the PR using GitHub's list of keywords
  • The appropriate reviewer is assigned to the PR
  • The appropriate developers are assigned to the PR
  • Pre-release package version incremented using usethis::use_dev_version()

Code Review

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

Code Review Checklist

  • I checked that new files follow naming conventions and are in the right place
  • I checked that documentation is complete, clear, and without typos
  • I added/edited comments to explain "why" not "how"
  • I checked that all new variable and function names follow naming conventions
  • I checked that new tests have been written for key business logic and/or bugs that this PR fixes
  • I checked that new tests address important edge cases

olivroy and others added 2 commits September 9, 2023 09:02
Update openxlsx2 lockfile version from 0.6 to 1.0, remove sep.names from tests that now produce warnings
@rsh52 rsh52 self-assigned this Sep 12, 2023
@rsh52 rsh52 added documentation Improvements or additions to documentation enhancement New feature or request labels Sep 12, 2023
@rsh52
Copy link
Collaborator

rsh52 commented Sep 12, 2023

@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.

@rsh52 rsh52 self-requested a review September 12, 2023 16:55
@rsh52 rsh52 merged commit d3cc563 into CHOP-CGTInformatics:main Sep 12, 2023
1 of 4 checks passed
@olivroy
Copy link
Contributor Author

olivroy commented Sep 12, 2023

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

@olivroy olivroy deleted the openxlsx2-deprecated branch September 12, 2023 17:11
@rsh52
Copy link
Collaborator

rsh52 commented Sep 12, 2023

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] openxlsx2 Deprecation Warning
2 participants