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

Windows Performance Improvement & Multi-Item support #43

Merged
6 commits merged into from
Jun 3, 2020
Merged

Windows Performance Improvement & Multi-Item support #43

6 commits merged into from
Jun 3, 2020

Conversation

arsenetar
Copy link
Owner

  • Use pywin32 to implement IFileOperation
  • Add ability to allow passing a list of files to send2trash across all platforms
  • Batch trash operations on windows for both IFileOperation and SHFileOperation

Notes:

  • My code format plugin formatted the contents of the files I changed, let me know if this is an issue and I can back those out (it fixed some flake8 style issues as well)
  • IFileOperation should open up the ability to address Windows backend can really delete files in some conditions #28 but I'll add in the extras for that in another PR.

arsenetar added 6 commits May 21, 2020 22:13
- Try using IFileOperation instead of SHFileOperation
  - Use pywin32 to accomplish this
  - Implement fallback when pywin32 not available
- Handles paths like `C:\` just fine bu the `\\?\` paths in the test
  cause issue
- Add batching for IFileOperation version (performance)
- Minor formatting applied by editor
- Strip these characters off if present just like old implementation
- Add check for windows version for IFileOperation
- Add list support to legacy version
- Remove some debugging code
- Fix bug in path converson

Not sure if there is a better way to layout this file
Formatter also ran on these so some other minor changes.
@BoboTiG
Copy link
Contributor

BoboTiG commented Jun 3, 2020

Wow this is quite a PR 💪 :)

Do you mind adding a test on Windows to force using the legacy implementation to be sure it is still tested?

@arsenetar
Copy link
Owner Author

Yeah it would be possible to try forcing the different versions, right now I am not sure which version the CI is actually testing, it looks like it is most likely still testing the legacy version, I'll see about getting the CI to install pywin32 and then test both versions.

@BoboTiG
Copy link
Contributor

BoboTiG commented Jun 3, 2020

FTR the CI is using Windows Server 2019, so it will likely test the new implementation only.

@arsenetar
Copy link
Owner Author

Either way, I should be able to get it to run both, with the dependency on pywin32 even though it is a new enough version of the OS it will not run without that package installed. I don't think the chocolatey python package includes anything different than the standard windows python installers, if that is the case it should be testing the legacy version based on what I have seen on Windows 10.

@ghost
Copy link

ghost commented Jun 3, 2020

@arsenetar this looks great and it's been a long time since these issues should have been tackled. You tackle them and it's excellent news.

As the README says, I'm looking for someone to take over this library. You're already doing a good job with dupeguru, would you mind taking this library too?

@arsenetar
Copy link
Owner Author

@hsoft I don't have any issues with taking this library over.

@ghost
Copy link

ghost commented Jun 3, 2020

Good! IIRC, we had problems during the last transfer because you had to delete your fork first. So I'll merge your PR right now and then let you delete your fork before proceeding.

Do you have a PyPI account? I'll transfer you publishing rights too.

@ghost ghost merged commit d078554 into arsenetar:master Jun 3, 2020
@arsenetar
Copy link
Owner Author

I have remove my fork. I did not have a PyPI account, I do now username is the same as here arsenetar.

@ghost
Copy link

ghost commented Jun 3, 2020

Transfer initiated. You've also been added as an owner of the pypi package.

@arsenetar
Copy link
Owner Author

Looks like both are good now.

@ghost
Copy link

ghost commented Jun 3, 2020

Good, godspeed! :)

This pull request was closed.
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.

2 participants