Modernize package w/ switch from {httr} to {httr2} #50
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- refactor: switch esriLayers to use esriRequest (replacing httr w/ httr2) - refactor: add is_urlServer helper function
- refactor: update esri2sf and esri2df to work w/ httr2 - feat: add geometry parameter to allow spatial filters other than bounding box objects (supported by new sf2geometry helper) - feat: add support to bbox parameter to convert sf objects to bbox (supported by new bbox2geometry helper) - refactor: rename bbox parameter to geometry for getEsriFeatures and other helper functions - refactor: switch replaceDomainInfo default to FALSE - refactor: disable esriUrl_isValid check for esrimeta - refactor: switch to use cli for messages and alerts - docs: separate esrimeta docs from esri2sf
- refactor: update NAMESPACE to export new functions + update imports - refactor: update DESCRIPTION to add magick and purrr to Suggests and cli, xml2 and httr2 to Imports
- refactor: add getLayerCRS helper function - refactor: switch default for outFields and where to NULL
- refactor: add support to bbox objects to sf2geometry (drops need for bbox2geometry helper) - refactor: add sf2geometryType helper function - refactor: add layerCRS as parameter to getLayerCRS function (only used to set default currently) - refactor: set outFields to NULL for esri2df and token to NULL for esrimeta
- fix: correct specification of `f` API parameter (required for implementation of ImageServer download functions that require both an f and format parameter) - docs: minor updates to multiple parameter definitions
|
I switched this pull request into a draft because I found a few critical issues (fixed now I think) and figured a more cautious approach would be better to make sure all outstanding issues are resolved before merging would be appropriate. Looking forward to your feedback @jacpete (or @yonghah if you have time to also take a look). |
- fix: add check for NULL values in field domain - refactor: set token default to NULL
- fix: correct parameter names in esriRequest - fix: use user-provided geomType if it is not NULL and warn if geomType does not match layerInfo$geometryType - fix: update esri2sf to correctly handle missing spatialReference values if crs is not NULL - fix: update extent2bbox to correctly reference min/max values
|
That latest commit improves support for layers with a missing spatial reference. Here is an example if you wanted to test/compare with the current version which cannot access this layer: https://services1.arcgis.com/mVFRs7NF4iFitgbY/arcgis/rest/services/21st_Century_Schools/FeatureServer/0 |
- refactor: add bbox2geometry and is_missing_geomType helper function, shorten line lengths, and use inherits for class checks - refactor: replace getWKTidAuthority w/ st_crs()$srid - docs: correct outdated parameter definition
- fix: make select for relocate more explicit in esriIndex - style: shorten line lengths in esriIndex.R - docs: minor updates to documentation
- fix: getEsriFeatures and getObjectIds must pass objectIds explicitly - fix: make sure layerInfo$type is not NULL before checking value in esri2df - fix: esriIndex reverse breaking change on relocate function
- docs: update README (note there is a possible outstanding issue with the polyline example)
- feat: add internal getbyIds param to getEsriFeatures to support accessing features w/o the interim step of getObjectIds - refactor: switch crs default to NULL for internal functions - fix: correct issue w/ getEsriFeaturesByIds using id instead of objectIds parameter The addition of the getbyIds parameter could be temporary or could be set as the new default behavior for queries below some size threshold.
- fix: switch to use httr2::req_body_form instead of httr2::req_body_json which simplifies the code and avoids the need for the getbyIds parameter for getEsriFeatures (maxed out at 1000 features anyway)
- docs: expand README w/ additional functions - feat: add lowercase name version of esriindex and esricatalog
- refactor: validate format arg and improve error message - docs: expand documentation
- feat: add quiet arg to optionally suppress messages - refactor: minor changes to messages
|
I figure it makes sense to close this pull request for now – but happy to re-open it in the future. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here is the summary of major changes in the pull request from #49
geometryparameter toesri2sf()to begin the process of allowing other types of spatial filters (not just bounding boxes)esriIndex()function described in Feature request: esriIndex function to list all folders and services on ArcGIS server #39esriInfo()function that includes a sitemap option that provides similar (but more limited) functionality as esriIndex