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

NodeishFilesystem rm needs force option #744

Closed
Tracked by #743
samuelstroschein opened this issue May 12, 2023 · 3 comments
Closed
Tracked by #743

NodeishFilesystem rm needs force option #744

samuelstroschein opened this issue May 12, 2023 · 3 comments
Assignees
Labels
scope: lix Related to source-code/git-sdk.

Comments

@samuelstroschein
Copy link
Member

Problem

The tests in #743 are failing because rmdir is used on a directory that contains files:

https://github.com/inlang/inlang/blob/6b8f7c65309b8c9645e68a27c85cda1b94fdc70c/source-code-git/fs/src/utilities/fromJson.test.ts#L10-L13

Using rmdir(path, { recursive: true }) is not an option because recursive: true is deprecated and (correctly) not exposed by NodeishFilesystem. Node recommends using rm(path, { recursive: true, force: true }) instead:

CleanShot 2023-05-12 at 17 02 50@2x

Proposal

NodeishFilesystem.rm needs the force option.

@samuelstroschein samuelstroschein added the scope: lix Related to source-code/git-sdk. label May 12, 2023
@samuelstroschein
Copy link
Member Author

PS man what an ugly API with way too many gotchas. But, let's get NodeishFilesystem in check first. Then, in a year or so introduce a sane Filesystem API.

@araknast
Copy link
Contributor

araknast commented May 13, 2023

As long as the directory is guaranteed to exist, you should be able to call await fs.rm(rootTempDir, { recursive: true }) to accomplish the desired behavior. If there is a possibility the directory does not exist you should be able to do something like await fs.rm(...).catch( e => e.code !== 'ENOENT' && throw e) to replicate the behavior of the force option.

@samuelstroschein
Copy link
Member Author

@araknast Yep, await fs.rm(rootTempDir, { recursive: true }) works! Again, what an ugly API. Eventually, we should have the force option. But, closing this issue until then.

@samuelstroschein samuelstroschein closed this as not planned Won't fix, can't repro, duplicate, stale May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: lix Related to source-code/git-sdk.
Projects
None yet
Development

No branches or pull requests

2 participants