Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Move dir optimization #550

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

Conversation

tfeldmann
Copy link
Contributor

@tfeldmann tfeldmann commented Aug 30, 2022

Type of changes

  • New feature
  • 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

  • Use shutil.move to move folders if both filesystems have a syspath
  • Check for ResourceNotFound, DirectoryExpected and IllegalDestination in fs.move.move_dir

@tfeldmann tfeldmann force-pushed the move_dir-optimization branch from fb1ba6b to 924107e Compare August 30, 2022 12:10
@coveralls
Copy link

coveralls commented Aug 30, 2022

Coverage Status

Coverage decreased (-0.0003%) to 94.814% when pulling 7c34e41 on tfeldmann:move_dir-optimization into 59f6e4d on PyFilesystem:master.

@tfeldmann tfeldmann marked this pull request as draft August 30, 2022 12:24
@tfeldmann
Copy link
Contributor Author

Once again I should rebase this for fewer commits. And it seems to have some problems on windows.

@tfeldmann tfeldmann force-pushed the move_dir-optimization branch from a2e1d25 to 475e6b8 Compare August 30, 2022 12:29
@tfeldmann tfeldmann force-pushed the move_dir-optimization branch from 475e6b8 to fb0b35a Compare August 30, 2022 12:46
@tfeldmann tfeldmann marked this pull request as ready for review August 30, 2022 13:30
@tfeldmann
Copy link
Contributor Author

This works now, but I'm kind of unhappy with it. For example I'm missing a param overwrite in move.move_dir. Is it ok for you if I add one? It would have to default to true to be backwards compatible.

with _src_fs.lock(), _dst_fs.lock():
with convert_os_errors("move_dir", dst_path, directory=True):
if _dst_fs.exists(dst_path):
os.rmdir(dst_syspath)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we don't need / want this os.rmdir?
Looking at the non-optimised "standard copy then delete" code, it looks like perhaps if dst_path already exists, and contains existing contents, then the existing contents will be preserved after the move_dir call? 🤷
Certainly something that it's probably worth adding an extra test for 😉

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants