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

skipping tests causing cran error, see comments in #205 #206

Merged
merged 2 commits into from
Nov 10, 2022
Merged

Conversation

geneorama
Copy link
Member

I don’t like punting and just skipping the tests, but I don’t know where I’m going to find that special dataset that is not updated, doesn’t risk changing order, and has only NULL values for the first 1,000 rows for at least one field.

And for the second test, I don’t even know what it was testing. It’s also hard to reconstruct without any example. It looks a little familiar and I think we may have developed the example together. I don’t recall though.

The new pull request passes CRAN tests documented in #205

@tomschenkjr
Copy link
Contributor

@geneorama
Copy link
Member Author

@tomschenkjr very good example, but I think we need the first 1000 rows to be NA.

Because of #184 the JSON will not return columns that are 100% NA.

I tried ordering the dataset by each column hoping that in one case the first 1000 rows would be 100% NA, but no dice.

Code to reproduce:

names <- c("school_name", "definition", "primary_address", 
           "second_address", "grades", "school2_name", "school2_primary_address", 
           "school2_secondary_address", "school2_grades", "school2_definition", 
           "school3_name", "school3_primary_address", "school3_secondary_address", 
           "school3_grades", "school3_definition", "school4_name", "school4_primary_address", 
           "school4_secondary_address", "school4_grades", "school4_definition")
urls <- sprintf("https://data.cityofchicago.org/resource/kqmn-byj8.json?$order=%s&$limit=1000", names)
dats <- lapply(urls, read.socrata)
lapply(dats, geneorama::NAsummary)
lapply(dats, dim)

@tomschenkjr
Copy link
Contributor

@geneorama Ah! That's right. Thanks for mentioning the underlying issue. I forgot it was completely missing columns on the first page of an API call.

@geneorama
Copy link
Member Author

@nicklucius Thanks for approving (@evejennings FYI)

I'll merge, build, and send to CRAN

@geneorama geneorama merged commit 2fa04b0 into master Nov 10, 2022
@geneorama geneorama deleted the issue205 branch November 10, 2022 18:50
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