Refactor to address multiple issues#197
Refactor to address multiple issues#197elipousson wants to merge 111 commits intowalkerke:masterfrom
Conversation
Run `roxygen2md::roxygen2md()` to apply markdown formatting
For consistency w/ docs + other functions
Also adds is_sf helper
- Add set_tigris_year and check_tigris_year functions to set default year + validate input year parameters - Add check_tigris_resolution to consolidate checks for resolution parameter - Add allow_null parameter to validate_state and validate_county - Reverse prior commit exposing the cb parameter in erase_water - Pass integer values to sprintf and substr (appears to work fine)
Also swap misnamed check_resolution for check_tigris_resolution
Simplifies logic in `input_to_wkt()` since filters were all converted to sfc regardless
Also add call parameter to internal helper functions (and start incorporating abort, warn, and inform from rlang) - set_tigris_year - check_tigris_year - input_to_wkt
Also: - drop uuid from Imports (unused) + add rlang to imports - update version + date to match main - add Eli Pousson as contributor
Note that tigris-package replaces tigris but tigris is used as alias
Also add pad_str helper function and replace str_trim w/ trimws (allows dropping stringr as dependency)
- Replace stop, warning, and message throughout - Use check_cb_year function to reduce duplication
Sets max year and default year to 2021 for core_based_statistical_areas
Adds validation for benchmark and vintage
Only check resolution in congressional_districts when cb=TRUE Also fix possible regression for state_legislative_district when cb=TRUE and year is not 2010 (url was being set and then over-written) Also validate year in voting_districts w/ min of 2012, max of 2020, and default of 2020
Use new arguments to improve checks w/in legislative functions
Use 2023 as new default year for current available ACS 5-year data
- Also update max year in check_tigris_year - Fill in missing param definitions for set_tigris_year - Add globalVariables from note
|
Awesome, tigris needs some updates and I think it's time to get this merged. |
`multiple` arg can't be set by user so shouldn't be referenced
Replace abort w/ cli_abort, warn w/ cli_warn, and inform w/ cli_inform Revise messages to follow cli inline syntax
Remove inform_vec and format_vec helper functions Fix validate_state to respect require_state argument on invalid input
Update erase_water to allow sfc class inputs for input_sf (faster for cartographic uses if input is unioned to a single object) and expose additional arguments w/ ... parameters Migrate to cli functions
Drop unecessary local_use_cli
|
Glad to hear you think that it is still possible to get this back into the main branch, @walkerke! Since there are so many changes in here already, I just went ahead and finished the migration to {cli} for all of the errors, warnings, and informational messages. I added the tests last year with intent to make it more feasible to merge while confirming that there hasn't been any regression. They are relatively basic (a combo of snapshot tests and tests to check errors and warnings). If you have any feedback on how you'd like the tests to be structured or configured, please let me know and I can do more on that. The only other major modernization task that I think may be beneficial is switching from httr to httr2 which would allow some further simplification of how the download URLs are built—but I'd rather see the bug fixes and quality of life improvements in this PR come back into the main package before trying to do anything more. |
|
Hi @elipousson - One thing that may impact where we go with this PR - the Census website is currently blocking all traffic to the TIGER/Line files (except when visited through a web browser), so tigris isn't working. Usually this resolves itself quickly, but it didn't change overnight, which makes me wonder if they've implemented some security policies that I wasn't aware of. I'll see what happens through the weekend and I've sent a note to Census, but we may need to consider an alternative approach for the package moving forward, as this is the second disruption in three months. |
|
Oh no! That is awful. And frustratingly inconvenient. I’d noticed some issues in the last day or so but evidently most of my data was cached so I thought it was weirdly specific to the 2023 area water data. We could rewrite the internals to use the ArcGIS REST services but the vintages are much more limited. There are still some small issues with the most recent batch of changes migrating to the cli package but I won’t bother fixing them until the data access question is resolved. |
|
@elipousson yeah, I don't know what to make of it yet. I'll hopefully have an answer this week and fingers crossed it's just a temporary thing so we don't need to make bigger changes. |
|
hey @elipousson - I'm going to merge my patch into the main branch, the issue still isn't resolved. It's not a huge change, it just adds a new I still want to get your PR in the package; I'd like to see what happens with this issue first though. |
|
No rush at all on my end. I'll follow the repo for further updates. |
2 participants
Re-opening a PR for this old PR after resolving some merge conflicts.
erase_water()should returninput_sfif there is no overlap w/ water area #172 (and duplicateerase_waterfails withyear2010, 2011, or 2012 #182)Additional user-facing changes are detailed in NEWS.