-
Notifications
You must be signed in to change notification settings - Fork 80
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
All functions in question appropriately fail or warn when file is missing #455
Conversation
expect_s3_class(x, c("tbl", "tbl_df", "data.frame")) | ||
expect_length(x, 18) | ||
expect_equal(nrow(x), 1) | ||
expect_equal(as.character(x$path), "missing") | ||
expect_equal(sum(is.na(x)), 17) | ||
}) | ||
it("produces error if a file does not exist and fail is set to TRUE", { | ||
expect_error(file_info("missing", fail = 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.
This is the key test for the new behaviour.
@@ -42,7 +42,7 @@ | |||
#' file_delete("mtcars.csv") | |||
#' \dontshow{setwd(.old_wd)} | |||
#' @export | |||
file_info <- function(path, fail = TRUE, follow = FALSE) { | |||
file_info <- function(path, fail = FALSE, follow = 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.
Keeping this as fail = TRUE
breaks {devtools}
, and for the sake of doing local development in RStudio, I changed this default, but this is something that needs to be discussed further.
This is too big of a change to be making at this point, at least for a contributor. |
Reprex
Closes #334
file_info()
Created on 2024-05-12 with reprex v2.1.0
file_size()
Created on 2024-05-12 with reprex v2.1.0
Concerns
@gaborcsardi Although this produces the desired behaviour, I think this is a massive change.
It is probably going to lead to breakages for packages that have been relying on the incorrect behaviour thus far: although the default for
{fs}
functions in question has always beenfail = TRUE
, these functions never failed, and they are now going to start to fail. I can already tell that{devtools}
is going to break with this change.To avoid this, should all defaults actually be changed from
fail = TRUE
tofail = FALSE
to keep this change backward-compatible?