Fixes #85 - posixify now works with JSON downloads #106
Conversation
|
I re-built the pull request in AppVeyor and it now passes. @geneorama - can you do a review of this pull request? Click "Add Your Review" to make any notes. It's for the v1.7.1 milestone that we'll submit on 10/31. |
| @@ -3,18 +3,29 @@ library(RSocrata) | |||
| library(httr) | |||
| library(jsonlite) | |||
| library(mime) | |||
| library(plyr) | |||
There was a problem hiding this comment.
plyr is not needed for the tests. If it's a package dependency it belongs in the appropriate places in the NAMESPACE and DESCRIPTION files.
There was a problem hiding this comment.
plyr is in NAMESPACE and DESCRIPTION so looks good to me.
| as.POSIXct(strptime(x, format="%m/%d/%Y")) # short date format | ||
| patternJSON <- paste0("^[[:digit:]]{4}-[[:digit:]]{2}-[[:digit:]]{2}T", | ||
| "[[:digit:]]{2}:[[:digit:]]{2}:[[:digit:]]{2}.[[:digit:]]{3}","$") | ||
| ## Find number of matches with grep |
There was a problem hiding this comment.
indenting (mentioned in commit message)
|
Thanks, Gene. The changes look good to me. |
| @@ -275,10 +281,12 @@ read.socrata <- function(url, app_token = NULL, email = NULL, password = NULL, | |||
| result <- rbind.fill(result, page) # accumulate | |||
There was a problem hiding this comment.
(We discussed this in the analytics meeting)
I'm not sure when this changed, but this depends on plyr, and would be more clear if it were fully specified with plyr::rbind.fill. Also, I don't see what problem this solves because if I revert this line to result <- rbind(result, page) all the tests still pass.
However, this is not part of the pull request.
|
I was going to do a "squash merge" but I don't want to lose authorship. @nicklucius would you mind doing that? In the commit message please take out my "removing plyr" and "adding plyr" messages. |
Fixes #85