RSocrata icon indicating copy to clipboard operation
RSocrata copied to clipboard

Allow user-defined offset

Open flynn-d opened this issue 6 years ago • 6 comments

Building off of #154, and also a prior PR #127, this change allows users to pass offsets as the queries. This skips paging and gives a message that the query has been offset.

I'll keep working on this if it breaks any of the current tests.

flynn-d avatar Oct 11 '18 02:10 flynn-d

Coverage Status

Coverage decreased (-0.5%) to 96.552% when pulling ab4dc578abe6ccdeab3a20db4ff17daa521ad69f on flynn-d:dev into 7cc5b303952ce526304f2bde703396ec430d7b54 on Chicago:dev.

coveralls avatar Oct 11 '18 02:10 coveralls

Error in the Travis CI build in the read.socrata examples... I can't figure out exactly what is going on, and can't seem to reproduce this on my forked version. Any tips?

> cleanEx()
Error: connections left open:
	httr::content(response, as = "text", type = "text/csv", encoding = "utf-8") (textConnection)
Execution halted

flynn-d avatar Oct 11 '18 02:10 flynn-d

@flynn-d - thank you for your pull request. This is very promising.

I re-ran the tests with slightly different errors:

2018-10-11 22:22:32.108 getResponse: Error in httr GET: 400  http://soda.demo.socrata.com/resource/4334-bgaj.csv?%24select=Region&$order=:id
2018-10-11 22:22:32.163 getResponse: Error in httr GET: 401  http://data.cityofchicago.org/resource/j8vp-2qpg.json?$order=:id
2018-10-11 22:22:39.130 getResponse: Error in httr GET: 400  https://soda.demo.socrata.com/resource/4334-bgaj.csv?%24app_token=ew2rEMuESuzWPqMkyPfOSGJgE&$order=:id
2018-10-11 22:22:39.778 getResponse: Error in httr GET: 400  https://soda.demo.socrata.com/resource/4334-bgaj.csv?%24app_token=ew2rEMuESuzWPqMkyPfOSGJgE&$order=:id
2018-10-11 22:22:42.339 getResponse: Error in httr GET: 403  https://soda.demo.socrata.com/resource/a9g2-feh2.csv?$order=:id
2018-10-11 22:22:42.901 getResponse: Error in httr GET: 403  https://soda.demo.socrata.com/resource/a9g2-feh2.json?$order=:id

All of them pertain to the order command. Is this not appearing on your local tests?

tomschenkjr avatar Oct 11 '18 22:10 tomschenkjr

Hi Tom -- As far as I can tell, these are all messages resulting from test which expect_error, and these warning messages appear. I ran devtools::test() on the master branch, and got the same messages as you see here. These don't appear to be why it failed (but might be additional issues also!).

Poking around, I saw the same cleanEx() problem show up in the master branch, too: https://travis-ci.org/Chicago/RSocrata/jobs/438836592.

I appreciate your guidance -- I'm finding this type of error for many packages testing on r-devel, e.g. search site:https://cran.rstudio.com/web/checks/ cleanEx connections left open, so I'm thinking it is a problem with r-devel-windows.... not sure what to do about that!

flynn-d avatar Oct 12 '18 01:10 flynn-d

@flynn-d - just getting to review the code (sorry for the delay). User offset is great, but should this feature be paired with user-defined limit as well? Right now, a hard-cap of 50,000 limit, so $offset tells the API which 50,000-lined page it loaded.

The 50,000 limit is because that's the maximum of the version 2.0 of the SODA API. However, version 2.1 does not have a limit.

Let me know if you think I'm off with my thinking on this.

tomschenkjr avatar Oct 26 '18 22:10 tomschenkjr

Tom, great idea, I'll work on that and update. Still unclear about the r-devel issue, but will give it a try.

flynn-d avatar Oct 27 '18 21:10 flynn-d