JonasKruckenberg / tauri-sys

Bindings to the Tauri API for projects using wasm-bindgen
Apache License 2.0
84 stars 21 forks source link

Implemented `fs` module #19

Closed bicarlsen closed 9 months ago

bicarlsen commented 1 year ago

I implemented most of the fs model adding the fs feature for it, with the exception of write_binary_file. I chose to accept options as function parameters instead using a builder pattern due to their simplicity.

I updated the README as well.

JonasKruckenberg commented 1 year ago

I don't mind the design without builders, they can indeed get quite unwieldy, however: I think we should mirror the existing std::fs API with our functions instead of rolling something custom, I also would like to avoid optional parameters as much as possible because they have terrible ergonomics in practice.

Example:

Instead of the current implementation of create_dir which must be called like this:

create_dir("some/path", BaseDirectory::Audio, Some(true)).await;

we should use the standard

create_dir_all("some/path", BaseDirectory::Audio).await;

pattern.

Edit: Or even better we could model the API off of cap_std::fs and have the following:

BaseDirectory::Audio::create_dir_all("some/path").await; // BaseDirectory::Audio would implement a `Dir` trait

// or alternatively

let dir = Dir::from(BaseDirectory::Audio)
dir.create_dir_all("some/path").await;
bicarlsen commented 1 year ago

I think we should mirror the existing std::fs API with our functions instead of rolling something custom

Not sure what you mean by this as I just copied the API from the fs API.

bicarlsen commented 1 year ago

I'm not familiar with the cap_std::fs library so skipped converting to that style for now, but am happy to dig into it a bit more if you think the effort is worth it.

bicarlsen commented 1 year ago

Just a couple things of note. You should also change all of these occurrences: .to_str().expect("could not convert path to str") to be non-panicking and instead return an error

What would the correct error to return be?

JonasKruckenberg commented 1 year ago

Just a couple things of note. You should also change all of these occurrences: .to_str().expect("could not convert path to str") to be non-panicking and instead return an error

What would the correct error to return be?

We don't have s variant for it yet, but it should be an Utf8 error, alternatively you can also use the to_string_lossy() method which doesn't fail

bicarlsen commented 1 year ago

@JonasKruckenberg Just wanted to bump this to see if there are any other changes to be made so it doesn't get too stale.

avsaase commented 1 year ago

What would be needed to move this PR forward?

JonasKruckenberg commented 9 months ago

Sorry people this repo has slipped from my radar for a while now (months???) but finally getting back into it. If you @bicarlsen wanna add those derive attributes before merging do that, otherwise we're finally ready I think!

Thanks for sticking with this for so long :)

bicarlsen commented 9 months ago

Should be good :)

Thanks @JonasKruckenberg