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

Use old readr on windows #206

Merged
merged 2 commits into from
Jul 21, 2021
Merged

Use old readr on windows #206

merged 2 commits into from
Jul 21, 2021

Conversation

agila5
Copy link
Collaborator

@agila5 agila5 commented Jul 20, 2021

TODO, just want to test the GHA. Will fix #205.

@agila5 agila5 marked this pull request as ready for review July 21, 2021 06:23
@agila5
Copy link
Collaborator Author

agila5 commented Jul 21, 2021

Tests have passed 🎉 Just one note: I decided to adopt edition 1 only on windows since the readr news says:

On Windows open files cannot be deleted as long as a process has the file open. Because readr keeps a file open when reading lazily this means you cannot read, then immediately delete the file.

This implies that the user gets slightly different results depending on the OS but, I think, the benefits of the new readr + vroom approach outweigh this difference. @Robinlovelace what do you think?

@agila5 agila5 requested a review from Robinlovelace July 21, 2021 06:28
@Robinlovelace
Copy link
Member

Looks great to me and +1 for using the newer and faster approach where possible.

@@ -120,10 +120,10 @@ dl_stats19 = function(year = NULL,
utils::download.file(zip_url, destfile = destfile, quiet = silent)
}
if(is_zip_file) {
f = file.path(destfile, utils::unzip(destfile, list = TRUE)$Name)
f2 = file.path(destfile, utils::unzip(destfile, list = TRUE)$Name)
Copy link
Member

Choose a reason for hiding this comment

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

Reasonable object renaming, sorry for the terse object names, and having thought about it more explicit and longer names would probably better - only issue being when you go over 80 characters on a line. FYI this would not be allowed if we used Rust and the clippy code checking tool. Heads up @dabreegster in case it's of interest, this is deep in the codebase that downloads, reads and cleans the STATS19 crash data for the UK.

@@ -27,6 +27,12 @@ read_accidents = function(year = NULL,
data_dir = get_data_directory(),
format = TRUE,
silent = FALSE) {
# Set the local edition for readr.
# See https://github.com/ropensci/stats19/issues/205
if (.Platform$OS.type == "windows" && utils::packageVersion("readr") >= "2.0.0") {
Copy link
Member

Choose a reason for hiding this comment

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

Minimising the impact of the change 👍

@Robinlovelace Robinlovelace merged commit 31a59d7 into master Jul 21, 2021
@Robinlovelace Robinlovelace deleted the unzip branch July 21, 2021 07:18
R/read.R Show resolved Hide resolved
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.

get_stats19() sometime fails when reading the same data twice
3 participants