v1.7.4 #149

tomschenkjr · 2017-12-11T23:08:19Z

This version fixes a bug raised by CRAN and introduces two very minor features. @nicklucius and @geneorama - please review the package of changes and add your approval or any comments.

New features:

Adds support for Socrata's grid view URL structure (e.g., ending with /data) (Invalid URL error when using "grid view" URL syntax #147)
read.socrata, write.socrata, and ls.socrata includes User-Agent in headers, allowing for Socrata to track RSocrata usage (from version 1.7.4 onward) (Set a custom HTTP User Agent header for tracking #119)

Bug fixes:

Temporary fix for Change unit test #137 by simply commenting-out the unit test calling to City of Boston. Future versions will provide an alternative fix for this.
Reduces "false positive" write.socrata() unit test by only checking for 200 HTTP status code (Rewrite write.socrata unit test to prevent false failures #143)
Solved erroneous error messages from unit testing (Resolve failing tests #148)

This test should only be temporarially ignored until a better solution (which does not compromise code coverage) is derived.

coveralls · 2017-12-11T23:13:24Z

Coverage increased (+0.5%) to 96.859% when pulling 9bbbeae on hotfix1.7.4 into 2c7d866 on master.

coveralls · 2017-12-11T23:13:24Z

Coverage increased (+0.5%) to 96.859% when pulling 9bbbeae on hotfix1.7.4 into 2c7d866 on master.

geneorama · 2017-12-12T16:45:39Z

I noticed that every time fetch_user_agent is called, it's called within httr::user_agent, like this: httr::user_agent(fetch_user_agent()).

Would it make more sense to bundle the httr::user_agent inside the fetch function?

geneorama · 2017-12-12T18:47:04Z

R/RSocrata.R

 #' @author Hugh J. Devlin, Ph. D. \email{Hugh.Devlin@@cityofchicago.org}
 #' @noRd
 getResponse <- function(url, email = NULL, password = NULL) {

  if(is.null(email) && is.null(password)){
-    response <- httr::GET(url)
+    response <- httr::GET(url, user_agent(fetch_user_agent()))


no httr:: in front of user_agent
(but this isn't an error)

Ok, agree with this. Will make the change (though strikes me as an odd style, it's semantically appropriate).

I wish I had never suggested the namespace thing because we don't consistently follow it (there are two places outside the review that don't have it). So, we get the downside of reduced readability without the benefit of having all foreign calls fully specified.

So I have mixed feelings about even making this comment.

I think it makes sense, the use of user_agent is odd because it--in my mind--only makes sense in the context of GET, POST, PUT, etc instead of a standalone function. It's a good idea to call the namespace (also, I think reduces the chance of us being chided by CRAN).

geneorama · 2017-12-12T18:47:33Z

R/RSocrata.R

 #' @export
 ls.socrata <- function(url) {
  url <- as.character(url)
  parsedUrl <- httr::parse_url(url)
  if(is.null(parsedUrl$scheme) | is.null(parsedUrl$hostname))
    stop(url, " does not appear to be a valid URL.")
  parsedUrl$path <- "data.json"
-  data_dot_json <- jsonlite::fromJSON(httr::build_url(parsedUrl))
+  #Download data
+  response <- httr::GET(httr::build_url(parsedUrl), user_agent(fetch_user_agent()))


no httr:: in front of user_agent
(but this isn't an error)

Agree, I'll make the change.

geneorama · 2017-12-12T18:49:31Z

R/RSocrata.R

+                    "R/", rVersion,
+                    ")"
+  )
+  return(header)


I would put the call to httr::user_agent here, but there is no error as it stands.
e.g. something like this:

result <- httr::user_agent(header) return(result)

I'm a little less inclined for this change. If we make the change, the call from GET would be:

GET( url, fetch_user_agent() )

But, I think I prefer the literal call-out in the current method:

GET( url, user_agent( fetch_user_agent() ) )

geneorama · 2017-12-12T18:52:59Z

tests/testthat/test-all.R

@@ -419,6 +419,17 @@ test_that("incorrect API Query Human Readable", {
  expect_equal(9, ncol(df), label="columns") 
 })

+context("URL suffixes from Socrata are handled")
+
+test_that("Handle /data suffix", {


Since we're only testing the ability to correctly parse URLs that end in data or data/ I would make the check just check that rather than checking the entire download process.

I'm inclined to agree. Can be a later touch-up.

/cc @nicklucius

geneorama · 2017-12-12T18:55:37Z

tests/testthat/test-all.R

-  na_time_rows <- df[is.na(df$TARGET_DT), ]
-  expect_equal(33, length(na_time_rows), label="rows with missing TARGET_DT dates")
-})
+# test_that("Read data with missing dates", { # See issue #24 & #27 


Note: we could

comment this to explain that the Boston data set we were using is no longer available

work to find a another similar test?

However this is fine as it stands

Yeah, good point, we should make a reference to the issue instead of just commenting-out. Will make the change.

geneorama

I have found no problems that would prevent merging this branch into master, but I have made some comments / observations.

tomschenkjr · 2017-12-12T19:08:20Z

Not sure if I follow bundling `user_agent` within `fetch_user_agent`. The latter grabs a bunch of system information but the former function prepares it to be sent as a header. Using `user_agent` within `GET` then sends that to the web server. Can't think of a way `user_agent` can be in the fetch function. Ideas? _ Tom Schenk Jr. Chief Data Officer Department of Innovation and Technology City of Chicago (312) 744-2770 tom.schenk@cityofchicago.org | @ChicagoCDO data.cityofchicago.org | opengrid.io | digital.cityofchicago.org | chicago.github.io | dev.cityofchicago.org

…

________________________________ From: Gene Leynes <notifications@github.com> Sent: Tuesday, December 12, 2017 11:45:39 AM To: Chicago/RSocrata Cc: Schenk, Tom; Assign Subject: Re: [Chicago/RSocrata] v1.7.4 (#149) I noticed that every time fetch_user_agent is called, it's called within httr::user_agent, like this: httr::user_agent(fetch_user_agent()). Would it make more sense to bundle the httr::user_agent inside the fetch function? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub<#149 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABkC0Qqx6XP-FvtAynlmsBXH1Gj2sXftks5s_q2zgaJpZM4Q-LA8>.

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

geneorama · 2017-12-12T19:22:38Z

I mean that I think you could return the header after you process it with user agent. So, each subsequent call to the function would be shorter and less nested. So, in the fetcher you'd have this at the end: return(httr::user_agent(header))

Right now each call looks like this: user_agent(fetch_user_agent())

With the proposed change each call would look like this: fetch_user_agent()

So, the fetch function would return the actual httr request object (which is a list of class "request") rather than a character string which needs to be converted to a request object each time.

@geneorama

Based on feedback from @geneorama, have included the `httr::` prefix to all `user_agent()` calls. Added clarifying comments related to #137 so our future-selves can be reminded of turning on the test again.

coveralls · 2017-12-12T20:18:38Z

Coverage increased (+0.5%) to 96.859% when pulling 53c4bf4 on hotfix1.7.4 into 2c7d866 on master.

tomschenkjr · 2017-12-12T20:19:48Z

I've made some changed based on Gene's feedback. Merging into master for now. We can make some corrections before we submit to CRAN, which I plan to do on Friday.

geneorama and others added 8 commits December 4, 2017 14:12

Only checking status code for write.socrata, closes #143

df43af1

Unit test for #147 - should fail

db5ecda

fixes #147

7b9ae5f

Ignoring test for #137

9a9377d

This test should only be temporarially ignored until a better solution (which does not compromise code coverage) is derived.

Version bump

bed76d5

Added User-Agent to read, write, and ls calls. Closes #119

48ce5f7

Updated documentation

05d004f

Added missing packages in namespace

9bbbeae

tomschenkjr self-assigned this Dec 11, 2017

tomschenkjr requested review from geneorama and nicklucius December 11, 2017 23:08

geneorama reviewed Dec 12, 2017

View reviewed changes

geneorama approved these changes Dec 12, 2017

View reviewed changes

Small syntax fixes; clarifying comments

53c4bf4

Based on feedback from @geneorama, have included the `httr::` prefix to all `user_agent()` calls. Added clarifying comments related to #137 so our future-selves can be reminded of turning on the test again.

tomschenkjr merged commit cff3e5f into master Dec 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v1.7.4 #149

v1.7.4 #149

tomschenkjr commented Dec 11, 2017

coveralls commented Dec 11, 2017

coveralls commented Dec 11, 2017

geneorama commented Dec 12, 2017

geneorama Dec 12, 2017

tomschenkjr Dec 12, 2017

geneorama Dec 12, 2017

tomschenkjr Dec 12, 2017

geneorama Dec 12, 2017

tomschenkjr Dec 12, 2017

geneorama Dec 12, 2017

tomschenkjr Dec 12, 2017

geneorama Dec 12, 2017

tomschenkjr Dec 12, 2017

geneorama Dec 12, 2017

tomschenkjr Dec 12, 2017

geneorama left a comment

tomschenkjr commented Dec 12, 2017 via email

geneorama commented Dec 12, 2017

coveralls commented Dec 12, 2017 •

edited

Loading

tomschenkjr commented Dec 12, 2017

v1.7.4 #149

v1.7.4 #149

Conversation

tomschenkjr commented Dec 11, 2017

coveralls commented Dec 11, 2017

coveralls commented Dec 11, 2017

geneorama commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geneorama left a comment

Choose a reason for hiding this comment

tomschenkjr commented Dec 12, 2017 via email

geneorama commented Dec 12, 2017

coveralls commented Dec 12, 2017 • edited Loading

tomschenkjr commented Dec 12, 2017

coveralls commented Dec 12, 2017 •

edited

Loading