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

Remove outdated dependencies; use testthat-3e #837

Merged
merged 5 commits into from
Mar 1, 2023

Conversation

jcheng5
Copy link
Member

@jcheng5 jcheng5 commented Dec 22, 2022

See #833

The code change in normalize-sp.R is assuming that r-spatial/sf#2069 gets merged. If not, either we'll need similar code, or else just warn/error that we don't handle this edge case. In either case, I need to write a test that exercises that code path.

PR task list:

  • Update NEWS
  • Add tests (where appropriate)
    • R code tests: tests/testthat/
    • Visual tests: R/zzz_viztest.R
  • Update documentation with devtools::document()

@jcheng5 jcheng5 marked this pull request as draft December 22, 2022 00:44
@jcheng5 jcheng5 marked this pull request as ready for review March 1, 2023 01:16
@jcheng5
Copy link
Member Author

jcheng5 commented Mar 1, 2023

@gadenbuie I added the unit test we discussed today; this is now ready to go. However, the rare case we discussed (sp objects that have holes but are missing the comment that indicates hole ownership) only works with sf 1.0-10, which is not yet on CRAN. If this case is encountered while the CRAN version of sf is installed, leaflet will report an error telling you to upgrade sf to 1.0-10 or above. The tests will pass either way, as I don't attempt the unit test if the wrong version of sf is installed.

I don't think we have to wait for sf 1.0-10 to hit CRAN to merge this PR, but I do think we should wait for that before we submit leaflet to CRAN.

@jcheng5 jcheng5 requested a review from gadenbuie March 1, 2023 04:24
@jcheng5 jcheng5 merged commit 668b6c0 into main Mar 1, 2023
@jcheng5 jcheng5 deleted the remove-retired-dependencies branch March 1, 2023 16:17
@jcheng5 jcheng5 mentioned this pull request Mar 9, 2023
3 tasks
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

Successfully merging this pull request may close these issues.

1 participant