Skip to content
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

feat(fs): improved API #751

Merged
merged 25 commits into from
Dec 20, 2023
Merged

feat(fs): improved API #751

merged 25 commits into from
Dec 20, 2023

Conversation

amrbashir
Copy link
Member

No description provided.

@amrbashir amrbashir requested a review from a team as a code owner November 21, 2023 02:42
@FabianLars
Copy link
Member

It doesn't compile with the watch feature enabled. Just a fairly small change though (the main thing is that resolve_path expects a SafePathBuf) but i won't push it to not risk conflicts if you have local commits left :)

(p.s. opened #761 to test more features in ci)

The main api change looks solid to me though (though i do think node's fs api is a bit weird idk), and what i tested so far worked nicely! 🥳 (i'll do some more testing tomorrow)
I do however think that this PR should have 2 approvals (maybe i just don't trust myself enough lol).

@amrbashir
Copy link
Member Author

Alright, watcher flag should be fine now, and feel free to push your changes if you need, I don't have any more changes planned.

though i do think node's fs api is a bit weird idk)

The API is inspired from Deno and Node and the main goal was to make it more familiar to those coming from node.

}
}

opts.read(options.read.unwrap_or(true))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they should all default to false (via serde instead of all the logic here); what if I want a file to be write-only for security reasons?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should I also change the default (the None case) behavior when options were not provided, to set read to false?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually we need to keep read to true, otherwise opening a file without read or write will fail.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if someone wants a write-only file, they will need to specify write: true, read: false

@FabianLars
Copy link
Member

Why was the close export removed? Isn't that required for open to make sense? (honestly already forgot everything about this pr so genuine question)

@amrbashir
Copy link
Member Author

I guess for security reasons, since close accepts any rid, I think they should use .close() on the returned FsFile class

@amrbashir
Copy link
Member Author

btw this PR is blocked until tauri-apps/tauri#8370

lucasfernog
lucasfernog previously approved these changes Dec 19, 2023
@amrbashir amrbashir force-pushed the feat/fs/improved-api branch from ff46b47 to ff17aae Compare December 19, 2023 23:14
@lucasfernog lucasfernog merged commit 69a1fa0 into v2 Dec 20, 2023
47 of 48 checks passed
@lucasfernog lucasfernog deleted the feat/fs/improved-api branch December 20, 2023 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants