walkerke
Refactor to address multiple issues
Re-opening a PR for this old PR after resolving some merge conflicts.
- Fixes #172 (and duplicate #182)
- Fixes #151
- Fixes #184 (error message may need further customization)
- Adds tests with testthat
- Add rlang and cli to imports
- Convert error messages to cli
- Drop unused Imports (stringr + uuid)
- Convert documentation to markdown formatting
Additional user-facing changes are detailed in NEWS.
Awesome, tigris needs some updates and I think it's time to get this merged.
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 protocol param to load_tiger() that allows users to point to the FTP site (working) or the HTTP site (currently not working). This could be modified to support the API as well in the future.
I still want to get your PR in the package; I'd like to see what happens with this issue first though.