fixes #15 - provide default sort order if not supplied in URL

[nicklucius]

If the user does not include an $order= in the URL, this will insert $order=:id.

There are no tests provided because this issue should only occur when the a dataset is being changed at the same time read.socrata() is paging.

fixes #15

[coveralls]

Coverage decreased (-0.7%) to 96.241% when pulling 4076f00 on issue15 into d6e307f on dev.

[geneorama]

I like the readability here.

Although, as I'm saying that, can I suggest this for the previous lines?

    if (!is.null(names(parsedUrl$query))) # check if URL has any queries
        ## if there is a query, check for $order within the query
        orderTest <- any(names(parsedUrl$query == "$order"))
        if(orderTest)
            ...

Although I love the trick of summing logicals to mean "or", I'm pretty sure that will work without sapply.

[nicklucius]

I tried substituting that code, but it broke a bunch of tests. I do like that its simpler and easier to read. I'll can try to tweak it and get it to work, since it looks like it should.

[nicklucius]

Got it to work and committed it. Thanks!

[tomschenkjr]

I think we need to include some tests to make sure this continues as expected in the future:

• Ensuring that the $order=:id is inserted when no other query parameters are used.
• Ensuring that $order=:id is not used when other $order parameters are requested by the user.
• Ensuring that $order=:id is inserted when other (non-$order) arguments are used.

[nicklucius]

I added some tests to ensure the expected behavior. I ended up having to add some code because I realized it wasn't adding $order=:id to clean URLs with no query parameters.

Please note that these tests ensure that the data frame contents are what we would expect if the URL is properly constructed. But they do not actually test against the URL itself, since that is only known to the function and not available to the tests scripts.

[tomschenkjr]

I never knew about that info option in test_that(). Great way to document!

From: Nick Lucius [mailto:notifications@github.com]
Sent: Wednesday, October 19, 2016 9:44 AM
To: Chicago/RSocrata RSocrata@noreply.github.com
Cc: Schenk, Tom Tom.Schenk@cityofchicago.org; Comment comment@noreply.github.com
Subject: Re: [Chicago/RSocrata] fixes #15 - provide default sort order if not supplied in URL (#109)

I added some tests to ensure the expected behavior. I ended up having to add some code because I realized it wasn't adding $order=:id to clean URLs with no query parameters.

Please note that these tests ensure that the data frame contents are what we would expect if the URL is properly constructed. But they do not actually test against the URL itself, since that is only known to the function and not available to the tests scripts.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHubhttps://github.com//pull/109#issuecomment-254834608, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABkC0eGeNm436HIHxghcFkqubI3p3v_gks5q1izGgaJpZM4KVHHi.

---

This e-mail, and any attachments thereto, is intended only for use by the addressee(s) named herein and may contain legally privileged and/or confidential information. If you are not the intended recipient of this e-mail (or the person responsible for delivering this document to the intended recipient), you are hereby notified that any dissemination, distribution, printing or copying of this e-mail, and any attachment thereto, is strictly prohibited. If you have received this e-mail in error, please respond to the individual sending the message, and permanently delete the original and any copy of any e-mail and printout thereof.

[tomschenkjr]

Great! Resolved all of my comments. @geneorama - anything else that should be considered before merging?