-
-
Notifications
You must be signed in to change notification settings - Fork 180
feat: add ArcGIS REST API fetch function #835
Conversation
This will need some work to function with the arcgis stuff currently in master. :-) |
Co-Authored-By: Larry Davis <lazdnet@gmail.com>
Co-Authored-By: Larry Davis <lazdnet@gmail.com>
Co-Authored-By: Emilio Graff <1@emil.io>
Co-Authored-By: Emilio Graff <1@emil.io>
Co-Authored-By: Emilio Graff <1@emil.io>
Co-Authored-By: Emilio Graff <1@emil.io>
Thank you for all these comments, second round of review is now open :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the coronadatascraper-cache update.
I believe that @shaperilio was looking at this as well. I'm reluctant to merge this currently, perhaps we can touch base tomorrow, @qgolsteyn , @shaperilio , and me.
I put a PR into @qgolsteyn's branch that migrates all the scrapers that would be affected. |
Fully migrate all scrapers to use your paginator
@qgolsteyn @jzohrab @camjc @lazd pinging you folks on this PR. @jzohrab , I believe you wanted @qgolsteyn to remove a submodule update? Note:
We need to set that to the date we want these sources to start using the paginated fetch. So it has to at least be the day that we merge. If scrape date is before this date, the scrapers will use their old URLs. I believe this can all go away with Testing: |
See #889 |
Closing this PR, as the work is part of #889. Thank you @qgolsteyn ! |
This PR creates a new function to fetch a JSON from an ArcGIS dashboard's REST API. It follows the suggestion from #832 by handling pagination, and the approach is compatible. I have update the SE scraper to demonstrate intended behavior. Any non-timeseries scraper will require a more significant change to avoid creating a cache-miss due to the changed URL.