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

Fix non recursive folder delete using file system #13361

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

dfriederich
Copy link
Contributor

@dfriederich dfriederich commented Feb 7, 2024

Fixes #13349

Deleting a folder without recursive enabled failed as the used nodejs API unlink does not support folder deletion. Fixing it by first checking if it is a folder, and if so using rmdir instead.

Added a simple unit test, which also exposed that
the folder deletion also fails when using trash support, but that is a different issue.

What it does

Handle the deletion of a folder using the file system api.
Previously this did only work when asking to delete the folders content as well, now with recursive: false,
it does delete the folder if it is empty.

How to test

I did add a unit test, which fails without the fix.

Follow-ups

I did notice that when using the trash support, the deletion of a folder with content (recursive: true) did fail.

Review checklist

Reminder for reviewers

Deleting a folder without recursive enabled failed as the used
nodejs API unlink does not support folder deletion.
Fixing it by first checking if it is a folder, and if so
using rmdir instead.

Added a simple unit test, which also exposed that
the folder deletion also fails when using trash support,
but that is a different issue.

Signed-off-by: Daniel Friederich <[email protected]>
@JonasHelming
Copy link
Contributor

@dfriederich Hi! If you mention "fixed #13349" in the description, an automatic link to the issue is created.

@msujew msujew added the filesystem issues related to the filesystem label Feb 7, 2024
@jfaltermeier jfaltermeier self-requested a review February 7, 2024 14:12
@dfriederich
Copy link
Contributor Author

Fixes #13349

You want me to change the description? Or I see you manually added the reference in this PR, was that sufficient?
If I change it, should I change the one commit and force push, or create a separate commit and then we squash the commit message when/if merging? Or can we just edit the commit message when merging?
Sorry, don't know how github is setup for theia.

@JonasHelming
Copy link
Contributor

Sorry for the confusion. What I meant is that if you mention "Fixes #xyz" in the description of the pull request (no necessarily the commit), Github will automatically link it on the issue. When I saw this PR I remembered that there was a corresponding issue. I searched for it and saw that you manually created a link over there so I thought, this information (auto linking) might be useful for you. We do not have strict rules about this, but traceability in both directions (PR<=>issue) is always good. Also, github will then automatically close the issue when this PR is merged.
Mentioning fixes #xyz is also good practice IMHO, but again we don't enforce this.

I believe Mark added the "fixes" tag in the beginning of the pull request description in the meantime.
I hope this makes sense! :-)

@dfriederich
Copy link
Contributor Author

@JonasHelming Thanks for the tip :-).

@jfaltermeier jfaltermeier merged commit 3034c0a into eclipse-theia:master Feb 9, 2024
14 checks passed
@jfaltermeier jfaltermeier added this to the 1.47.0 milestone Feb 9, 2024
@dfriederich dfriederich deleted the folder_delete branch February 9, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filesystem issues related to the filesystem
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Theia's file service delete function fails to erase empty folders without recursive option
4 participants