Skip to content

Add replaceFile #200

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add replaceFile #200

wants to merge 1 commit into from

Conversation

zlonast
Copy link

@zlonast zlonast commented May 17, 2025 •
edited
Loading

@zlonast zlonast mentioned this pull request May 17, 2025
Draft
@zlonast zlonast changed the title Add replaceFile [Draft] Add replaceFile May 17, 2025
@zlonast zlonast force-pushed the master branch 4 times, most recently from d74c63b to 9c9dc0e Compare May 17, 2025 21:04
Bodigrim
Bodigrim reviewed May 18, 2025
View reviewed changes
System/Directory/OsPath.hs
@@ -709,6 +710,30 @@ renamePath opath npath =
(`ioeAddLocation` "renamePath") `modifyIOError` do
renamePathInternal opath npath

-- | 'replaceFile' replaces one file with another file. The replacement file
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to elaborate explicitly how replaceFile is different from renameFile.

Copy link
Author

Choose a reason for hiding this comment

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

Added more description

Copy link

@noughtmare noughtmare May 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think the documentation would help me much more if you add something like: "In constrast to renameFile, replaceFile ... (is atomic?) You should use this when ... For example, consider this program ..."

In other words, the current explanation raises these questions with me:

  • What do renamePath and ReplaceFileW do? (Perhaps this is just meant for people who are familiar with those functions and I should just ignore that sentence.)
  • What can go wrong if I use renameFile? Why is it important that it is atomic?

System/Directory/OsPath.hs
@@ -52,6 +52,7 @@ module System.Directory.OsPath
, copyFile
, copyFileWithMetadata
, getFileSize
, replaceFile
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the convention is to provide the same set of functions both from System.Directory and from System.Directory.OsPath.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@zlonast zlonast force-pushed the master branch 2 times, most recently from 22cf56c to cb53340 Compare May 18, 2025 15:22
@zlonast zlonast marked this pull request as ready for review May 18, 2025 15:26
@zlonast zlonast changed the title [Draft] Add replaceFile Add replaceFile May 18, 2025
@Rufflewind
Copy link
Member

@Rufflewind Rufflewind added the blocked: upstream Progress cannot be made until an upstream issue is resolved. label May 21, 2025
@noughtmare
Copy link

As an outsider, it seems that replaceFile should always be preferred over renameFile, because it is guaranteed to be atomic on all platforms. Would it make sense to eventually replace the implementation of renameFile with this new replaceFile function? Would that be a backwards compatible change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bodigrim Bodigrim Bodigrim left review comments

@noughtmare noughtmare noughtmare left review comments

Assignees
No one assigned
Labels
blocked: upstream Progress cannot be made until an upstream issue is resolved.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zlonast @Rufflewind @noughtmare @Bodigrim