-
Notifications
You must be signed in to change notification settings - Fork 26
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
Convert delete_all_canonicalised to delete folders as a unit #102
Conversation
Not quite sure why the Windows test failed, it passed on my system. |
Seems like the issue with ignoring the second argument was actually with how I was calling into the function. Now working perfectly fine! I was deleting items individually and calling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for figuring this out!
It seems like this was working all the time then, even though I have the feeling I am missing something.
Maybe @ArturKovacs could have a look just to be sure I am not committing an ecosystem-breaking mistake here 😅.
Otherwise, ready to go from my side.
I think that the current tests should cover any potential regressions although I'm not sure. Looking at the change it seems fine, but then I don't even remember what these functions do. All things considered I'm okay with merging this. |
I must say I haven't tested this code in a super robust way. I'm calling into it from an existing project that depends on this crate. I suppose that does have it's advantages in that my application uses basically all the functionality of this library, and so far I haven't noticed any new issues. I haven't been systematic however. |
I ran the tests on Windows and they were successful.
…On Tue, Mar 12, 2024, 08:47 Emma Alexandria ***@***.***> wrote:
I must say I haven't tested this code in a super robust way. I'm calling
into it from an existing project that depends on this crate. However, so
far I haven't noticed any significant issues.
—
Reply to this email directly, view it on GitHub
<#102 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB7PKCE67Q7W5IEFXOCIXA3YX2XIXAVCNFSM6AAAAABERCJBK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJQHE3DOOJSG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Since I validated that the tests cover this case, I'd think that new new way of doing things will be a general improvement as on Windows there won't be individual items in the trash-bin anymore after deleting a directory. Thank @ArturKovacs, then it looks like if there was a deeper meaning behind the now changed behaviour, it's lost in the tides of time. Even though maybe on older Windows versions the new code doesn't work, I think it's fine to assume it will and if there are bug-reports, this new version can be yanked. It's beta-testing with users, essentially… and while writing this I realize it's probably better to declare it a braking change to allow applications to opt-in (while hoping they read the changelog). |
The new release can be found here: https://github.com/Byron/trash-rs/releases/tag/v4.0.0 . |
Draft to solve #101. Doesn't seem to be handling multiple arguments properly (e.g ignoring second argument)