Skip to content

Conversation

@elipousson
Copy link
Contributor

@elipousson elipousson commented May 21, 2025

This is the first in what is likely going to be a series of PRs to pull the relevant and useful feature additions from this conflicted and over-large PR #197

Changes in this PR:

  • Refactor validate_state()+ validate_county() with the addition of various helper functions
  • Export validate_state()+ validate_county() with new multiple argument
  • Add {rlang} and {cli} to Imports
  • Add {withr} to Suggests
  • Refactor input_to_wkt() function to allow sfc input objects and warn if object contains multiple geometries after sf::st_union is applied
  • Expand test coverage
  • Update NEWS with highlights of these changes

Related #136

Add rlang and cli to Imports

Also refactor input_to_wkt function to:

- allow sfc input
- warn if object contains multiple geometries after union is applied
Also expose geos_method argument internally
Add withr to Suggests and set edition for testthat

Correct test for lookup_code
@elipousson elipousson changed the title Export validate_state()+ validate_county() Export validate_state()+ validate_county() and add tests May 27, 2025
@walkerke
Copy link
Owner

Thanks Eli! It'll be helpful to go through this incrementally. Just let me know when it's ready for review.

@elipousson
Copy link
Contributor Author

elipousson commented May 27, 2025

@walkerke I think this it is ready now! I caught a typo while testing the branch this morning so I decided to pull in the tests from my original PR and make sure I hadn't introduced any other breaking bugs. The tests should also make it easier to review any additional PRs since they will catch any major regressions.

I will say thought that the tests are pretty basic so feel free to suggest changes if you have any approaches you prefer.

@elipousson
Copy link
Contributor Author

If the tests continue to fail unpredictably with GitHub Actions, I can add skip_on_ci() and the tests can be run locally alone. They work fine on my computer but I think they may be running into some rate-limiting if they can't use the cache.

@walkerke
Copy link
Owner

I will keep an eye on it! I'll get this fully reviewed shortly.

@walkerke walkerke merged commit 611fc19 into walkerke:master Jul 18, 2025
0 of 5 checks passed
@walkerke
Copy link
Owner

Thanks Eli! I do see that some tests are failing - I think this will be useful for monitoring performance locally, but I'm adding skip_on_ci() and skip_on_cran() as certain Census shapefiles go down from time to time.

@elipousson
Copy link
Contributor Author

That sounds like a good approach! I can open a second PR that pulls the new validation functions into all of the data access functions. I'm still working my way through the changes from that old, overloaded PR and hoping that the tests will make all of the remaining changes much easier to review.

@walkerke
Copy link
Owner

sounds good! I also added you as a package contributor in DESCRIPTION, I put the email I could find on the web but go ahead and change it to a different one if you want

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.

2 participants