Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fixes #19 JSON download error #102

Merged
merged 2 commits into from
Oct 5, 2016
Merged

Fixes #19 JSON download error #102

merged 2 commits into from
Oct 5, 2016

Conversation

nicklucius
Copy link
Contributor

I think this closes #19. It uses fromJSON to parse the download and then uses rbind.fill from plyr to deal with the NAs and missing columns in the JSON files.

…deal with missing columns in JSON files.

Changes JSON download tests because now JSON files download logical data as a logical data type, instead of character.
@tomschenkjr
Copy link
Contributor

Haven't done a full code review yet, but a few things.

The unit testing failed. Looks like the rbind.fill() was one of those reasons. Probably because it's relying on plyr, which means the NAMESPACE needs to be updated to include it. Otherwise, R won't install the dependencies. Also, add it to the DESCRIPTION file under Imports.

Also, be sure to sign the Contributor License Agreement.

@coveralls
Copy link

coveralls commented Oct 4, 2016

Coverage Status

Coverage remained the same at 96.899% when pulling 8f7d057 on issue19 into 1e65927 on dev.

Copy link
Contributor

@tomschenkjr tomschenkjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. I didn't understand the need to include "logical" on the expect_equal.

@@ -125,7 +125,7 @@ test_that("read Socrata JSON as default", {
expect_equal(1007, nrow(df), label="rows")
expect_equal(11, ncol(df), label="columns")
expect_equal(c("character", "character", "character", "character", "character",
"character", "character", "character", "character", "character",
"character", "logical", "character", "character", "character",
Copy link
Contributor

@tomschenkjr tomschenkjr Oct 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the "logical" return now needed? Is it for the needs_recoding field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is for needs_recoding. It had been coming in as a "character" but with this code change it started coming in as a "logical". I think using fromJSON() is what caused this change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but we'll have to be very careful to note this since this could mess-up someone's existing code. That is, someone downloading from Socrata may have code that works because that field is downloaded as chr, but may not because it's logical.

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

Successfully merging this pull request may close these issues.

3 participants