added: restore_single TrashItem that can restore to arbitrary destination and force restore #138
Conversation
There was a problem hiding this comment.
Thanks a lot for the initiative.
In its current form I don't think the forceful removal will be usable by anyone just yet unless this is wired up on the top-level. There it would also only be available for freedesktop users I presume.
Personally I also find myself avoiding bare booleans in favor of enums that clearly say what they do when in argument position.
|
I can try to make it work and a introduce top-level api under |
|
At first, I thought I can just do a small refactor like I did for freedesktop, but to my limited knowledge, its seems each restore operation depends on a The function signature would look something like this: pub fn restore_single(item: &TrashItem, restore_options: &RestoreOptions) -> Result<(), Error>;, with the struct looking like this: pub struct RestoreOptions {
// generic
destination: Path,
force: bool,
// specific
pfo: IFileOperation,
...
}Later on @Byron thought? |
|
Having OS-limited functions would be a way forward - that way you could just avoid platforms that you don't specifically need. |
jx-00
changed the title
|
This is done if I am not missing anything?
|
| @@ -343,6 +343,13 @@ pub struct TrashItemMetadata { | |||
| pub size: TrashItemSize, | |||
| } | |||
|
|
|||
| #[derive(Debug, Clone, Copy, Default)] | |||
| pub enum RestoreMode { | |||
There was a problem hiding this comment.
This isn't documented at all. Could you add #![deny(missing_docs)] to lib.rs? That way this would light up in CI.
| @@ -343,6 +343,13 @@ pub struct TrashItemMetadata { | |||
| pub size: TrashItemSize, | |||
| } | |||
|
|
|||
| #[derive(Debug, Clone, Copy, Default)] | |||
| pub enum RestoreMode { | |||
| Force, | |||
There was a problem hiding this comment.
The variants could probably be less abstract by spelling out more closely what they do.
| /// } | ||
| /// ``` | ||
| /// | ||
| /// [`RestoreCollision`]: Error::RestoreCollision | ||
| /// [`RestoreTwins`]: Error::RestoreTwins | ||
| pub fn restore_all<I>(items: I) -> Result<(), Error> | ||
| pub fn restore_all<I>(items: I, mode: RestoreMode) -> Result<(), Error> |
There was a problem hiding this comment.
I don't think this can be a breaking change, after all this is only implemented on one platform.
Further, I was expecting that restore_single() is exposed on a single platform, right now it doesn't seem available at all.
There was a problem hiding this comment.
Yeah, I totally agree. I went this route because I noticed that the same functions on Windows and Freedesktop are routed to os_limited. Since I’m not super familiar with Windows, I didn’t feel confident implementing restore_single() for it, so I decided to change the argument instead to keep things consistent.
I did consider adding configuration attributes specifically for Freedesktop, but since there wasn’t a precedent for that, I wasn’t quite sure how to approach it.
Some guidance on this would be much appreciated!
There was a problem hiding this comment.
Some guidance on this would be much appreciated!
Undo the public API change and instead expose restore_single() in an os-limited way, just for the platform it's actually implemented in. This is new to this crate and you'd have to add such capability and probably a test as well (for the public API) just to be sure it's actually accessible.
|
Sorry, if anything it's probably something I did wrong. I'm still new to working non solo project. I'm trying to learn what I can. |
I need the ability to restore to arbitrary destination and force restore, for my one of my project.
This also addresses a todo:
Cheers.