pyfilesystem2 icon indicating copy to clipboard operation
pyfilesystem2 copied to clipboard

Published 20 hours ago • verified publisher icon PyFilesystem

Fix backward incompatibility introduced in 2.4.16 (#535)

Open tfeldmann opened this issue 3 years ago • 10 comments

Type of changes

  • Bug fix
  • Tests

Checklist

  • [x] I've run the latest black with default args on new code.
  • [x] I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
  • [x] I've added tests for new code.
  • [x] I accept that @PyFilesystem/maintainers may be pedantic in the code review.

Description

Added overwrite=True to the optimized path in fs.move.move_file. move_file now behaves like in versions <2.4.16.

tfeldmann avatar Jul 07 '22 13:07 tfeldmann

Coverage Status

Coverage remained the same at 94.814% when pulling 93c2579f14fab4261c45178ccf5689e05f326426 on tfeldmann:master into 11ad1ecd591a3b56329712e4f3fec51ab808ba2d on PyFilesystem:master.

coveralls avatar Jul 07 '22 13:07 coveralls

I found another case where the optimized and the regular code path behave differently:

with open_fs(fs_url) as tmp:
    tmp.writetext("file.txt", "content")
    fs.move.move_file(tmp, "file.txt", tmp, "file.txt")

If you move a file onto itself, the optimized code path works as expected, but the regular code path happens to delete the file. This is not wanted, I guess? I added some code to fix this.

EDIT: I found this happens also if you do some_fs.move("file.txt", "file.txt", overwrite=True). It deletes the file. Is this expected?

tfeldmann avatar Jul 08 '22 09:07 tfeldmann

Eeek! If attempting to move a file onto itself, I guess it makes sense to just early-exit? :thinking:

lurch avatar Jul 08 '22 12:07 lurch

What stops this PR from being resolved? @althonos @tfeldmann

Wallaku avatar Jul 21 '22 08:07 Wallaku

I still think you ought to do normpath on src_path and dst_path before comparing them, in case they contain relative-paths like foo/../bar ? (also, you'll need to rebase this PR to resolve the conflicts)

lurch avatar Jul 21 '22 09:07 lurch

Yep, some things left to do on my side. Didn't have the time yet.

tfeldmann avatar Jul 21 '22 09:07 tfeldmann

I found this happens also if you do some_fs.move("file.txt", "file.txt", overwrite=True). It deletes the file.

@tfeldmann Can you create an issue for https://github.com/PyFilesystem/pyfilesystem2/blob/master/fs/base.py#L1138 please?

lurch avatar Jul 21 '22 11:07 lurch

@tfeldmann Can you create an issue for https://github.com/PyFilesystem/pyfilesystem2/blob/master/fs/base.py#L1138 please?

On it!

tfeldmann avatar Jul 21 '22 11:07 tfeldmann

HOORAY! Good job everybody let's bump a version for this library already! @lurch

Wallaku avatar Jul 21 '22 14:07 Wallaku

Anyone? @lurch @althonos

Wallaku avatar Jul 31 '22 08:07 Wallaku

Great job @tfeldmann !

althonos avatar Aug 19 '22 09:08 althonos