Use references in restore_all and purge_all

[oberblastmeister]

restore_all and purge_all should take references because they don't need the ownership of TrashItems. This could reduce cloning when calling the functions. restore_all might need an additional clone on a RestoreCollision error because there is remaining_items. My use case is specified in #53 .

[Byron]

Thanks a lot, I agree!

A PR would be very welcome - I'd appreciate maintaining compatibility by using generics, i.e. where Iter: IntoIterator<Item = T>, T: AsRef<TrashItem> or something along these lines, if at all possible.

[TD-Sky]

Is it necessary to save remaining_items when the parameter of these functions are I: IntoIterator<Item = &TrashItem>?
We save it in current version because this is the only way to get back the items. Are there any other reasons?

[Byron]

What does 'Is it necessary to save remaining_items' mean? I don't think I understand the meaning of save here. Thanks for the clarification.

[TD-Sky]

I think the type of parameter affects the error handling.

For example, we don't need field items in Error::RestoreTwins anymore because of function restore_all(Iter<&TrashItem>) can neither modify nor take the list of TrashItem.

I'm confused with should remaining_items be kept in Error. After all, we would always have the list of TrashItem when functions taking only reference.

[Byron]

I understand that the current API might be sub-optimal as it is affected by passing ownership - if references were passed, the API could be simplified.

With that in mind, I think it would be possible to sketch this, and a PR for that would be very welcome.
Thanks everybody for bringing this up.

[TD-Sky]

I have some questions about the doc of Error::RestoreCollision v3.0.1:

One should not assume any relationship between the order that the items were supplied and the list of remaining items. That is to say, it may be that the item that collided was in the middle of the provided list but the remaining items’ list contains all the provided items.

Does it mean returning the number of restored files (like returning the number of bytes read/written in I/O) is completely unfeasible?

But I do make it and the unit test for linux passed:

• New RestoreCollision: https://github.com/TD-Sky/trash-rs/blob/4c34bf948b25db266744e0870a4c03e097d3d0a2/src/lib.rs#L178
• The unit test: https://github.com/TD-Sky/trash-rs/blob/4c34bf948b25db266744e0870a4c03e097d3d0a2/src/tests.rs#L168

---

Actually I have finished all the work for the coming PR. Now I'm rebuilding them carefully to get a neat git commits record and a clear changelog. So there would be more commits after 2023-03-12 UTC+0 possibly.

I feel sorry for asking you frequently before pulling request, but I have to make sure that I'm doing the effective work. Thanks.

[Byron]

No worries. Since you are interested in neat commit histories, you might be interested in Stacked Git - I use it myself and it's super useful build a decent commit history from the get-go, which distributes the time it takes to do that through the development process.

Also CC @ArturKovacs as the original author of the above quote.

[ArturKovacs]

One should not assume any relationship between the order that the items were supplied and the list of remaining items. That is to say, it may be that the item that collided was in the middle of the provided list but the remaining items’ list contains all the provided items.

Does it mean returning the number of restored files (like returning the number of bytes read/written in I/O) is completely unfeasible?

No. It just means that the order of remaining_items is unspecified. With v3.0.0 the user could calculate the number of restored items like this (I haven't tested the code below, also I suggest being careful with it on your actual computer, as it will attempt to restore all trashed items)

let items = trash::list().unwrap();
let target_count = items.len();
if let Error::RestoreCollision{remaining_items, ..} = trash::restore_all(items) {
    let restored_count = target_count - remaining_items.len()
}

Obviously if restore_all will only take a reference of the items, then this error cannot store owned TrashItems. It could store a Vec<&'a TrashItem>. Then you obviously also have to add 'a to the error enum Error<'a> {. Also feel free to improve the wording of the documentation.

[TD-Sky]

One should not assume any relationship between the order that the items were supplied and the list of remaining items. That is to say, it may be that the item that collided was in the middle of the provided list but the remaining items’ list contains all the provided items.

Does it mean returning the number of restored files (like returning the number of bytes read/written in I/O) is completely unfeasible?

No. It just means that the order of remaining_items is unspecified. With v3.0.0 the user could calculate the number of restored items like this (I haven't tested the code below, also I suggest being careful with it on your actual computer, as it will attempt to restore all trashed items)

let items = trash::list().unwrap();
let target_count = items.len();
if let Error::RestoreCollision{remaining_items, ..} = trash::restore_all(items) {
    let restored_count = target_count - remaining_items.len()
}

Obviously if restore_all will only take a reference of the items, then this error cannot store owned TrashItems. It could store a Vec<&'a TrashItem>. Then you obviously also have to add 'a to the error enum Error<'a> {. Also feel free to improve the wording of the documentation.

The purpose of returning restored: usize in RestoreCollision is to allow users could continue restoring items:

let items = trash::list().unwrap();
if let Error::RestoreCollison {restored, ..} = trash::os_limited::restore_all(&items) {
    trash::os_limited::restore_all(items.iter().skip(restored + 1)).unwrap();
}

I wrote the similar code in the unit test as mentioned above and it has passed. However, the error would be unfeasible if the restoration order isn't the provided order. It may lead the item that has been restored be restored again. This is the core of my question.

[TD-Sky]

Hey, @ArturKovacs , I wonder if #65 can be completed. Could you answer my question above?

[ArturKovacs]

I wrote a comment under the PR.

[TD-Sky]

@oberblastmeister Would you like to close this issue? Because the behavior discussed was determined now.

---

@Byron (off-topic) You forgot to update the version in README.