Refactor to fix `erase_water()` #172 + #151 + #184 + improve error checking w/ rlang

[elipousson]

This is the updated scope of changes in this pull request:

• Fixes #172 (and likely duplicate #182)
• Fixes #151
• Fixes #184 (error message may need further customization)
• Adds tests for around half of functions (expect to expand next week)
• Improve formatting of error messages
• Drop unused Imports (stringr + uuid)

[elipousson]

After catching the two outdated default years in erase_water() and coastline(), I realized that it might be easier to maintain the package if you didn't need to update that number in so many places.

After a close look at the code, I think I was able to consolidate as of the basic year validation into two small helper functions (set_tigris_year() and check_tigris_year()). I also eliminated the distinction between the cyear variable and the year variable since sprintf and substr both seem to handle integer inputs fine and check_tigris_year is now handling validation so it should always be a valid integer.

I also added the allow_null parameter to validate_state() and validate_county() to remove duplicative error checking code and the check_tigris_resolution() helper function to do the same thing.

Hope this doesn't complicate review for the pull request! Let me know if you want me to make an changes or restart with a clean pull request that only addresses the issue with erase_water().

[elipousson]

The one other thing worth mentioning is that since you are already importing {dplyr} it may be worth swapping all of your stop() functions for cli::cli_abort() or rlang::abort().

You could also potentially integrate the standalone input checking functions that can be imported with usethis::use_standalone("r-lib/rlang", "types-check").

I've adopted those type checking functions in several packages now and they are really handy – happy to open an issue if you wanted to discuss or make the changes on this pull request if it is a welcome suggestion.

[walkerke]

Awesome! A lot of the tigris infrastructure is old, dating back to when I first wrote the package several years ago. I like what you are doing to modernize it. I should have some time to review later this week.

[elipousson]

I caught a typo in my original pull request but couldn't leave well enough alone and did some more (hopefully welcome) refactoring. I noticed there was a lot of duplication in the way the TIGER URLs were built so I added a helper function url_tiger() that I think makes the URL logic a bit easier to read.

I also spotted that input_to_wkt() could be modified to support sfc input object pretty easily (both bbox and sf objects were converted to sfc before being converted to well-known text) so I went ahead and made that tweak as well.

[elipousson]

I went a bit over-board for what started as a small pull request but I'm hoping to review and address the merge conflicts next week. The hopefully welcome addition is that I'm building out the tests to double-check that the refactored error checking is all working. I'll update the PR description and name with a better summary.

I think it actually should be pretty helpful in making maintenance easier but I'd welcome feedback or suggestions.

[walkerke]

Thanks @elipousson! This solves a number of lingering issues and will help to modernize the package / make it easier to update. I'll pull down this PR and take a look through it.

[elipousson]

Awesome. There may be a couple of outstanding issues. You'll see in the tests some that are commented out – those are mostly tests that I thought should work that didn't! I'm not sure whether it is changes on the Census FTP server side or just me mis-reading what each function supports but figured I'd flag those for your consideration.

It also may still be simpler to add cli as a direct dependency rather than using rlang for all the warnings and errors. If that sounds good to you, I'm happy to make the change.

[walkerke]

This is excellent and I'll pull down and review - this will really modernize the package.

Let me know when you think it's ready to merge. I don't mind using rlang for warnings / errors.

[elipousson]

I just committed the couple of outstanding local changes:

• A helper to check URLs for errors per #185
• Refactoring (similar to elsewhere in this PR) for load_tiger() including the addition of a get_tigris_cache_dir helper
• Improved checks for filter_by inputs

I think anything else can wait for a future PR so this should be ready for review, @walkerke

[elipousson]

I caught a regression (which is why several tests that I had expected to pass were all failing) in the checks for minimum years and fixed an additional issue created by missing 2022 geography for metro areas. I also spotted your suggestions in #129 for places data for 2000 so implemented that as well.

I filled in a few more tests for enumeration units so the overall test coverage is up to 38%. Admittedly, not the best tests but still hopefully should make the review a little less painful.

[elipousson]

I thought it was ready for review before but now I really think it is ready for review.

I updated NEWS to include a summary of changes in this PR (but not a full summary of all changes since the 2.0.0 version).

I also continued to expand test coverage up to >50%. This was helpful in catching some regressions or bugs in the new features.

[elipousson]

@walkerke Did you have a chance to take a look at this?

Happy to make further changes or just open a new PR with a more limited scope if you wanted part but not all of these changes. I could open a PR with just the tests, for example, and then you could use them to make sure the larger PR doesn't break anything that works with the existing package as it is. Thanks for your work on this package as always!

[walkerke]

Thanks @elipousson - just need to get a couple projects behind me and I should be able to go through this in early June. Thanks for your work on this!

[elipousson]

Thanks for the update! I’d seen online that you had a ton of workshops and events this spring but wanted to make sure I didn’t cross wires with your review if I caught any more bugs or adding any more tests.