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
Don’t overwrite existing file if appending an empty data frame #451
Conversation
I recommend turning off altrep / lazy reading in this test. |
append
before returning empty file
@jennybc I believe this is ready for another look! |
tests/testthat/test-vroom_write.R
Outdated
expect_equal( | ||
vroom_lines(tf, | ||
altrep = FALSE, | ||
skip_empty_rows = FALSE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see why we need to specify skip_empty_rows
.
If we don't, let's remove it. In which case, the expectation can be come more compact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need it. I just wanted to make sure it wasn't writing an empty line when vroom_write(data.frame(), file = file, append = TRUE)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if vroom_write(data.frame(), file = file, append = TRUE)
wrote an empty line, that would be a bug and we would want to know about it. So I think the test is better without it (which I see has happened now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick:
let's remove it. In which case, the expectation can be come more compact.
There's still this.
Co-authored-by: Jennifer (Jenny) Bryan <jenny.f.bryan@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the expectation in the new test more similar to other in this file / package.
I added a NEWS bullet.
Fixes tidyverse/readr#1408
Previously, if
vroom_write()
was supplied an empty data frame, vroom created an empty file without checking the value ofappend
. But this meant data from the file could be deleted such as in this case.Created on 2022-07-15 by the reprex package (v2.0.1.9000)