New API: borrow TrashItems

[TD-Sky]

This PR brings the following new features:

• restore_all and purge_all take &TrashItem as parameters instead of taking ownership. It helps us avoid cloning TrashItems when we want to keep the ownership of them after restoring or purging.

• Correspondingly, the Error::RestoreCollision now contains field restored: usize which means how many items are restored before collision. Anyone who wants to continue restoring can skip restored + 1 items on the passed iterator.

However, I don't understand why the doc tells me One should not assume any relationship between the order that the items were supplied and the list of remaining items but I passed the unit test for RestoreCollison.

• Error is constructed by thiserror now. The reason for using thiserror is there are too many I/O Result in the codes. #[error(transparent)] Io(#[from] io::Error) makes the error handling more convenient and reasonable.

Sadly, this PR only works on Linux. I can't setup a virtual machine of other platforms temporarily. If this PR is verified to be sensible, I wish someone could complete the work of other platforms.

[Byron]

Thank you, the linux implementation looks like a reasonable improvement, I think as discussed prior in #54.

Adding thiserror probably isn't an issue as it's part of most dependency trees anyway, despite it being heavy for trees that don't have any proc-macros.

If @ArturKovacs thinks it makes sense as well maybe we can see this through.

[TD-Sky]

One more feature I forgot to explain:

• The broken symbolic link is regarded as existing entity. trash-rs will panic at assert!(file.exists()) because of broken symbolic link before this PR. Now restoring (actually moving) or purging (actually removing) broken symbolic link won't fail anymore. But restoring item may collide with broken symbolic link now.

[TD-Sky]

I decide to contains the path where the I/O error occurred in Error::Io. Only then can we print error message like "{path}: {error}".

[ArturKovacs]

It's been a while I wrote code with this crate, and I haven't run into the problem discussed here, but if I understand correctly, then this PR isn't the best solution for the issue.

I don't think there's a need for the change introduced in this PR, I think you can just use the current API to solve this problem.

For example this code will re-try the restoration, but during the second try, it skips the trash item that couldn't be restored. (Note that I haven't tested this code and it will attempt to restore all your trashed files.)

let items = trash::list().unwrap();
if let Err(RestoreCollision {path, remaining_items}) = trash::os_limited::restore_all(items) {
    // keep all except the one that couldn't be restored.
    remaining_items.retain(|e| e.original_path() != path);
    trash::os_limited::restore_all(remaining_items)
}

[TD-Sky]

It's been a while I wrote code with this crate, and I haven't run into the problem discussed here, but if I understand correctly, then this PR isn't the best solution for the issue.

I don't think there's a need for the change introduced in this PR, I think you can just use the current API to solve this problem.

For example this code will re-try the restoration, but during the second try, it skips the trash item that couldn't be restored. (Note that I haven't tested this code and it will attempt to restore all your trashed files.)

let items = trash::list().unwrap();
if let Err(RestoreCollision {path, remaining_items}) = trash::os_limited::restore_all(items) {
    // keep all except the one that couldn't be restored.
    remaining_items.retain(|e| e.original_path() != path);
    trash::os_limited::restore_all(remaining_items)
}

Borrow references of TrashItem and return count of the restored ones could do the same thing with lower overhead if the restoration order keeps up with the references:

    let items = trash::list().unwrap();
    if let Err(RestoreCollision { path, restored }) = trash::os_limited::restore_all(&items) {
        // keep all except the one that couldn't be restored.
        let items = items
            .iter()
            // you can skip restored + 1 in this show snippet, that's OK
            .skip(restored)
            .filter(|e| e.original_path() != path);
        trash::os_limited::restore_all();
    }

Now the problem is, there are no contracts about it, in other words, there is a contract tells users shouldn't depend on the order:

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.

I wonder why.

[Byron]

Thanks for chiming in @ArturKovacs and providing an example.

@TD-Sky Performance isn't ever an issue until it is measured, so I don't think borrowing for that reason carries a lot of value. what do you think about making this PR mergeable only by adding a non-running documentation example instead, based on @ArturKovacs example? It would show how to retry without breaking the API.

[TD-Sky]

Thanks for chiming in @ArturKovacs and providing an example.

@TD-Sky Performance isn't ever an issue until it is measured, so I don't think borrowing for that reason carries a lot of value. what do you think about making this PR mergeable only by adding a non-running documentation example instead, based on @ArturKovacs example? It would show how to retry without breaking the API.

Are there any other ways to implement #54?

[Byron]

Seeing how resolving this has come along, I think it's worth it to consider cutting losses and rejecting the premise. Even though reference could be used, they aren't, and cloning the keep them isn't known to be a performance issue.

There might be error cases where the input items are lost, and one might consider to 'return' the owned items in the error similar to how it was already demonstrated. This seems to be in the interest of the original author and wouldn't break the API for most, and I see value in that.

[TD-Sky]

Seeing how resolving this has come along, I think it's worth it to consider cutting losses and rejecting the premise. Even though reference could be used, they aren't, and cloning the keep them isn't known to be a performance issue.

There might be error cases where the input items are lost, and one might consider to 'return' the owned items in the error similar to how it was already demonstrated. This seems to be in the interest of the original author and wouldn't break the API for most, and I see value in that.

So you thought it better to pass in references and return owned items when error occurs? I have to apologize for my potential misunderstanding, because I'm not a native English speaker.

[Byron]

The original author indicated that references and changing the API there isn't needed to solve the problem.
The documentation could be improved instead to show how to work with such an owning API, which might include suggesting to clone the input so that retries are possible.
There is a new test here which could be rewritten to work with the owning API as well, I presume.

Now that we have direction, I hope this PR can get the work it needs to be merged.
Thank you.

[TD-Sky]

I wanna close this PR and open a new one because the correct direction disagrees the topic of this PR.

[TD-Sky]

Wait, can we make purge_all accept references? It has no the problem of restore_all.

[Byron]

From what I can tell, the whole premise of "References are needed" is not only put into question but rejected. It would be a breaking change and despite being more idiomatic to use references if no consumption happens internally, changing it now half-way wouldn't change much effectively except breaking the consistency of the crate API, which overall is consuming in nature.

It seems to be in the interest of the @ArturKovacs to leave it as is, and I suggest to look at specific problems with the API and add documentation and examples to show how these can be solved.

[TD-Sky]

From what I can tell, the whole premise of "References are needed" is not only put into question but rejected. It would be a breaking change and despite being more idiomatic to use references if no consumption happens internally, changing it now half-way wouldn't change much effectively except breaking the consistency of the crate API, which overall is consuming in nature.

It seems to be in the interest of the @ArturKovacs to leave it as is, and I suggest to look at specific problems with the API and add documentation and examples to show how these can be solved.

But purge_all could have a compatibility update by changing API to IntoIterator<Item=T> where T: AsRef<TrashItem>.

[Byron]

But purge_all could have a compatibility update by changing API to IntoIterator<Item=T> where T: AsRef<TrashItem>

Great idea - if purge_all isn't cloning internally then, this should be very doable.