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

Fix backward incompatibility introduced in 2.4.16 (#535) #542

Merged
merged 9 commits into from
Aug 19, 2022

Conversation

tfeldmann
Copy link
Contributor

Type of changes

  • Bug fix
  • Tests

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
  • I've added tests for new code.
  • 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.

@coveralls
Copy link

coveralls commented Jul 7, 2022

Coverage Status

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

tests/test_move.py Outdated Show resolved Hide resolved
@tfeldmann
Copy link
Contributor Author

tfeldmann commented Jul 8, 2022

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?

@lurch
Copy link
Contributor

lurch commented Jul 8, 2022

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

fs/move.py Outdated Show resolved Hide resolved
@Wallaku
Copy link

Wallaku commented Jul 21, 2022

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

@lurch
Copy link
Contributor

lurch commented Jul 21, 2022

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)

@tfeldmann
Copy link
Contributor Author

tfeldmann commented Jul 21, 2022

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

@lurch
Copy link
Contributor

lurch commented Jul 21, 2022

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?

Copy link
Contributor

@lurch lurch left a comment

Choose a reason for hiding this comment

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

👍

@tfeldmann
Copy link
Contributor Author

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

On it!

@Wallaku
Copy link

Wallaku commented Jul 21, 2022

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

@Wallaku
Copy link

Wallaku commented Jul 31, 2022

Anyone?
@lurch
@althonos

fs/move.py Outdated Show resolved Hide resolved
@tfeldmann tfeldmann requested a review from althonos August 16, 2022 07:27
@althonos althonos merged commit 59f6e4d into PyFilesystem:master Aug 19, 2022
@althonos
Copy link
Member

Great job @tfeldmann !

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.

5 participants