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

Small change to prevent converting literal "NA"s as missing. #244

Merged
merged 2 commits into from
Nov 23, 2021

Conversation

jmobrien
Copy link
Collaborator

Set type_convert() to use na = character() to avoid converting anything further to NA (all suitable NAs should have been identified by the read_csv() call just above.)

@juliasilge
Copy link
Collaborator

I wonder if it would be better to use the dots ... here for all arguments passed to type_convert(), and then we make ... on fetch_survey() for most of the arguments that pass through to read_survey() like col_types, etc.

@jmobrien
Copy link
Collaborator Author

Yeah, I had similar general thoughts, but I think a fixed argument works best for this specific issue. Looks like the endpoint only ever offers up "" for truly missing data (ignoring seen-but-missing). Unless I'm reading this wrong, setting those ""'s to NA is already covered by the read_csv() call on line 115. So, adding na = character() to type_convert just eliminates any extra missingness checks, which I think is what we want.

Considering the potential for something like ..., the potentially-flexible args for type_convert are col_types, locale, and trim_ws. We have col_types already, but only partly support locale through time_zone, and nothing for trim_ws. ... could offer consolidation and add support for whitespace removal. However, it's worth noting that the central role for the standalone time_zone arg in fetch_survey() is as a param for the API query.

So, I'm not sure. Though, I am thinking this PR is just a bugfix that can be considered separately.

@juliasilge juliasilge merged commit a2c65de into ropensci:master Nov 23, 2021
@juliasilge
Copy link
Collaborator

Thank you so much @jmobrien! 🙌

@jmobrien jmobrien deleted the literalNAfix branch November 23, 2021 20:32
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.

None yet

2 participants