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

add the param version to write_rds() #1001

Merged
merged 3 commits into from Oct 22, 2019
Merged

Conversation

shrektan
Copy link
Contributor

@shrektan shrektan commented Jun 12, 2019

Since serialization format 3 is the default on R 3.6.0, our users may want specify the rds serialization format they are using, due to the fact that format 3 can't be read by R version before 3.5.0.

@jennybc
Copy link
Member

jennybc commented Jun 12, 2019

For context, when faced with a related situation in usethis::use_data(), I chose to expose the version but default to 2. It's worth considering whether readr should do same.

https://usethis.r-lib.org/reference/use_data.html

r-lib/usethis@12392c5

@jimhester
Copy link
Member

jimhester commented Oct 17, 2019

I think that we should default to 2 as well here, as it has greater compatibility.

Can you please add a bullet to NEWS? It should briefly describe the change and end with (@yourname, #issuenumber).

Also could you re-run the roxygen documentation and commit the results, either devtools::document() or roxygen2::roxygenize().

Otherwise this looks good, thanks!

@shrektan
Copy link
Contributor Author

shrektan commented Oct 20, 2019

@jimhester I've changed the default value of version to 2, run the docs and added an item in NEWS. Thanks. :D

@shrektan
Copy link
Contributor Author

shrektan commented Oct 22, 2019

BTW, the failed checks are caused by other reasons and not related to this PR.

image

@jimhester
Copy link
Member

jimhester commented Oct 22, 2019

A great improvement!

Thanks a million!

@jimhester jimhester merged commit b73dbbc into tidyverse:master Oct 22, 2019
@shrektan shrektan deleted the rds-version branch Oct 22, 2019
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

3 participants