Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Allow user-defined offset #157

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Allow user-defined offset #157

wants to merge 2 commits into from

Conversation

flynn-d
Copy link

@flynn-d flynn-d commented Oct 11, 2018

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.

@coveralls
Copy link

coveralls commented Oct 11, 2018

Coverage Status

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

@flynn-d
Copy link
Author

flynn-d commented Oct 11, 2018

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

@tomschenkjr
Copy link
Contributor

@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?

@flynn-d
Copy link
Author

flynn-d commented Oct 12, 2018

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!

@tomschenkjr tomschenkjr self-requested a review October 26, 2018 21:09
@tomschenkjr
Copy link
Contributor

@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.

@flynn-d
Copy link
Author

flynn-d commented Oct 27, 2018

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants