Conversation
|
|
||
|
|
||
|
|
||
| ############################################################################### |
There was a problem hiding this comment.
If this is necessary I think it's time to start a new file! Please create a file folders.py and import AtomicFolder from it.
There was a problem hiding this comment.
Sure, I'll do that once the content is fine!
| """ | ||
| if os.path.isdir(dst): | ||
| raise FolderAlreadyExists("'{}' already exists".format(dst)) | ||
| shutil.move(path, dst) |
There was a problem hiding this comment.
This function is not atomic as the implementation in luigi notes in its warning message. Here is a race condition for example: We have two processes A and B, which both run this function.
- Process A runs
isdir, which returns false - Process B runs
isdir, which returns false - Process A moves its tmpdir to the target path
- Process B moves its tmpdir to the target path (or rather, into the directory created by A)
I am not sure whether we can guarantee to fail atomically if the target path doesn't exist. If you have any ideas regarding that... Ideally we would have an overwrite param but I am not sure if that is possible.
| class FolderAlreadyExists(Exception): pass | ||
|
|
||
|
|
||
| class AtomicFolder(): |
There was a problem hiding this comment.
Please model AtomicFolder like the API of AtomicWriter for consistency, concretely that it itself is not a contextmanager but rather has a open() method that returns a context manager, and separate methods for commit and rollback.
There was a problem hiding this comment.
Oh and please subclass from object such that we have a newstyle class on both Python 2 and 3.
|
See discussion on #35 |
2 participants
This is pretty much just copy&paste from one of my projects. Your version is much more sophisticated but I think it's good to have the code just to get the discussion started.
Things that have to be addressed:
rename_dont_movecould be replaced by existing functionality.@untitaker what do you think?
I'll clean up the commit history once the PR is ready.
This PR closes #35.