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

Make it explicit that deleteFile will delete a folder recursively #52941

Closed
joaomoreno opened this issue Jun 26, 2018 · 4 comments
Closed

Make it explicit that deleteFile will delete a folder recursively #52941

joaomoreno opened this issue Jun 26, 2018 · 4 comments
Assignees
Labels
feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded workspace-edit
Milestone

Comments

@joaomoreno
Copy link
Member

joaomoreno commented Jun 26, 2018

Testing #52800

The docs of deleteFile should be explicit in that a folder will be deleted recursively, since that's the opposite of the usual FS APIs out there in which a folder can only be removed (rmdir) if it's empty.

@jrieken
Copy link
Member

jrieken commented Jun 26, 2018

Yeah, we should maybe do what already have done for file system providers. Add an option options: { recursive: boolean }

@jrieken jrieken added workspace-edit polish Cleanup and polish issue labels Jun 26, 2018
@jrieken jrieken added this to the June 2018 milestone Jun 26, 2018
@jrieken jrieken assigned bpasero and unassigned jrieken Jun 27, 2018
@jrieken
Copy link
Member

jrieken commented Jun 27, 2018

@bpasero I have updated the API and make sure the data travels to the bulk-edit-service but it seems that the textfile-service doesn't know about recursive folder delete

@bpasero
Copy link
Member

bpasero commented Jun 27, 2018

@jrieken that's right because the IFileService does not expose any option to not delete recursively. I am not sure there would be any other client, so why not assert this within the bulkEditService and bail out early if the folder is not empty, similar to how it already bails out if the file exists: https://github.com/Microsoft/vscode/blob/39b91f6c674882ef20fbde20b95ed6c01cd3a576/src/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.ts#L345

@jrieken
Copy link
Member

jrieken commented Jun 27, 2018

I'd say that once we expose the full 'reading'-api for file system provider we will encounter this again (see #48034). Sure, we can do a short term trick in the bulk edit service but I think the fileservice/textfileservice-couple is the place that should have this. Likely also true for ignoreIfExists (which we might nuke again...)

bpasero added a commit that referenced this issue Jun 28, 2018
…53205)

* Make it explicit that `deleteFile` will delete a folder recursively (#52941)

* add FileDeleteOptions to IFileSystemProvider#delete
@bpasero bpasero added verification-needed Verification of issue is requested feature-request Request for new features or functionality and removed polish Cleanup and polish issue labels Jun 28, 2018
@bpasero bpasero closed this as completed Jun 28, 2018
@roblourens roblourens added the verified Verification succeeded label Aug 1, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Aug 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded workspace-edit
Projects
None yet
Development

No branches or pull requests

4 participants