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

Return File Path after Writing in fwrite #6181

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Return File Path after Writing in fwrite #6181

wants to merge 8 commits into from

Conversation

Nj221102
Copy link
Contributor

Issue

closes #5706.

Description

The current implementation of fwrite returns an invisible value upon completion, which can be limiting in scenarios where users need to programmatically access the path of the created file for subsequent operations. This pull request aims to enhance the functionality of fwrite by modifying it to return the path of the file it writes to.

Changes Made

  • Functionality Enhancement: Updated fwrite to return the file path after writing data to disk.
  • Updated tests accordingly.

@Rdatatable Rdatatable deleted a comment from github-actions bot Jun 14, 2024
Copy link

github-actions bot commented Jun 14, 2024

Comparison Plot

Generated via commit f0e5510

Download link for the artifact containing the test results: ↓ atime-results.zip

Time taken to finish the standard R installation steps: 12 minutes and 43 seconds

Time taken to run atime::atime_pkg on the tests: 3 minutes and 36 seconds

NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@@ -84,11 +84,11 @@ fwrite = function(x, file="", append=FALSE, quote="auto",
if (file.exists(file)) {
suggested <- if (append) "" else gettextf("\nIf you intended to overwrite the file at %s with an empty one, please use file.remove first.", file)
warningf("Input has no columns; doing nothing.%s", suggested)
return(invisible())
return(invisible(file))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe here NULL should be returned, since the actual file is not affected at all by fwrite()? WDYT @eliocamp

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a though one. Honestly this behaviour is kind of unexpected to me. I would've expected fwrite to either write an empty file or to throw an error.

With append == TRUE, the file is left untouched, which would be the expected behaviour when writing an empty data.table, so returning the file would make sense, though.

@@ -9977,7 +9977,7 @@ test(1658.31, fwrite(ok_dt, qmethod=c("double", "double")), error="length(qmetho
test(1658.32, fwrite(ok_dt, col.names="foobar"), error="isTRUEorFALSE(col.names)")

# null data table (no columns)
test(1658.33, fwrite(data.table(NULL)), NULL, warning="Nothing to write")
test(1658.33, fwrite(data.table(NULL)), "", warning="Nothing to write")
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, wonder how useful this case is too -- when fwrite() to the terminal, maybe NULL should be returned as well? Not sure any inconsistency is worthwhile for this case though.

@tdhock
Copy link
Member

tdhock commented Jun 15, 2024

I like returning NULL instead of file name in some cases, so user can test to see if anything was written

@MichaelChirico
Copy link
Member

I like returning NULL instead of file name in some cases, so user can test to see if anything was written

That was my initial thought, though Elio points out it might be a bit awkward in the append= case. I'm imagining a pipeline like

do_stuff() |>
  # ...
  last_step() |>
  fwrite(file, append=TRUE) |>
  manipulate_file() |>
  more_stuff()

Needing to ensure the file was written in order to get something useful out seems inconvenient here -- the file exists, after all.

(might not be the clearest explanation, I don't have a 100% clear thought here yet, but instinctually it's like the above)

Maybe we can do "return file path" ↔️ "the path exists on exit"?

@eliocamp
Copy link
Contributor

It's also awkward when saving within apply or even inside j

DT[, fwrite(.SD), by = groups]

Returning NULL would essentially drop groups, wouldn't it?

In general, I feel it's kind of weird when functions return different things without any active decision from the user. It makes the function unpredictable and harder to program with.

My ideal behaviour would be to always save whatever is being saved (even if its an empty data.table) and return the file path. For this PR, I don't think we can change what is saved, so at least for now I think a good solution would be to return the path if the file actually represents what's being saved so only returning NULL in the case of trying to write an empty data.table to an exiting file with append = FALSE.

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.

fwrite() should (possibly invisibly) return the file path
5 participants