Make it explicit that deleteFile will delete a folder recursively (#…

…53205) * Make it explicit that `deleteFile` will delete a folder recursively (#52941) * add FileDeleteOptions to IFileSystemProvider#delete
microsoft · Jun 28, 2018 · a908be0 · a908be0
1 parent 405f8be
commit a908be0
Show file tree

Hide file tree

Showing 12 changed files with 70 additions and 34 deletions.
diff --git a/src/vs/platform/files/common/files.ts b/src/vs/platform/files/common/files.ts
@@ -128,10 +128,11 @@ export interface IFileService {
 	createFolder(resource: URI): TPromise<IFileStat>;
 
 	/**
-	 * Deletes the provided file.  The optional useTrash parameter allows to
-	 * move the file to trash.
+	 * Deletes the provided file. The optional useTrash parameter allows to
+	 * move the file to trash. The optional recursive parameter allows to delete
+	 * non-empty folders recursively.
 	 */
-	del(resource: URI, useTrash?: boolean): TPromise<void>;
+	del(resource: URI, options?: { useTrash?: boolean, recursive?: boolean }): TPromise<void>;
 
 	/**
 	 * Allows to start a watcher that reports file change events on the provided resource.
@@ -158,6 +159,10 @@ export interface FileWriteOptions {
 	create: boolean;
 }
 
+export interface FileDeleteOptions {
+	recursive: boolean;
+}
+
 export enum FileType {
 	Unknown = 0,
 	File = 1,
@@ -196,7 +201,7 @@ export interface IFileSystemProvider {
 	stat(resource: URI): TPromise<IStat>;
 	mkdir(resource: URI): TPromise<void>;
 	readdir(resource: URI): TPromise<[string, FileType][]>;
-	delete(resource: URI): TPromise<void>;
+	delete(resource: URI, opts: FileDeleteOptions): TPromise<void>;
 
 	rename(from: URI, to: URI, opts: FileOverwriteOptions): TPromise<void>;
 	copy?(from: URI, to: URI, opts: FileOverwriteOptions): TPromise<void>;

diff --git a/src/vs/workbench/api/electron-browser/mainThreadFileSystem.ts b/src/vs/workbench/api/electron-browser/mainThreadFileSystem.ts
@@ -8,7 +8,7 @@ import { Emitter, Event } from 'vs/base/common/event';
 import { IDisposable, dispose } from 'vs/base/common/lifecycle';
 import URI from 'vs/base/common/uri';
 import { TPromise } from 'vs/base/common/winjs.base';
-import { FileWriteOptions, FileSystemProviderCapabilities, IFileChange, IFileService, IFileSystemProvider, IStat, IWatchOptions, FileType, FileOverwriteOptions } from 'vs/platform/files/common/files';
+import { FileWriteOptions, FileSystemProviderCapabilities, IFileChange, IFileService, IFileSystemProvider, IStat, IWatchOptions, FileType, FileOverwriteOptions, FileDeleteOptions } from 'vs/platform/files/common/files';
 import { extHostNamedCustomer } from 'vs/workbench/api/electron-browser/extHostCustomers';
 import { ExtHostContext, ExtHostFileSystemShape, IExtHostContext, IFileChangeDto, MainContext, MainThreadFileSystemShape } from '../node/extHost.protocol';
 
@@ -107,8 +107,8 @@ class RemoteFileSystemProvider implements IFileSystemProvider {
 		return this._proxy.$writeFile(this._handle, resource, encoded, opts);
 	}
 
-	delete(resource: URI): TPromise<void, any> {
-		return this._proxy.$delete(this._handle, resource);
+	delete(resource: URI, opts: FileDeleteOptions): TPromise<void, any> {
+		return this._proxy.$delete(this._handle, resource, opts);
 	}
 
 	mkdir(resource: URI): TPromise<void, any> {

diff --git a/src/vs/workbench/api/node/extHost.protocol.ts b/src/vs/workbench/api/node/extHost.protocol.ts
@@ -42,7 +42,7 @@ import { ITreeItem } from 'vs/workbench/common/views';
 import { ThemeColor } from 'vs/platform/theme/common/themeService';
 import { IDisposable } from 'vs/base/common/lifecycle';
 import { SerializedError } from 'vs/base/common/errors';
-import { IStat, FileChangeType, IWatchOptions, FileSystemProviderCapabilities, FileWriteOptions, FileType, FileOverwriteOptions } from 'vs/platform/files/common/files';
+import { IStat, FileChangeType, IWatchOptions, FileSystemProviderCapabilities, FileWriteOptions, FileType, FileOverwriteOptions, FileDeleteOptions } from 'vs/platform/files/common/files';
 import { ConfigurationScope } from 'vs/platform/configuration/common/configurationRegistry';
 import { CommentRule, CharacterPair, EnterAction } from 'vs/editor/common/modes/languageConfiguration';
 import { ISingleEditOperation } from 'vs/editor/common/model';
@@ -681,7 +681,7 @@ export interface ExtHostFileSystemShape {
 	$rename(handle: number, resource: UriComponents, target: UriComponents, opts: FileOverwriteOptions): TPromise<void>;
 	$copy(handle: number, resource: UriComponents, target: UriComponents, opts: FileOverwriteOptions): TPromise<void>;
 	$mkdir(handle: number, resource: UriComponents): TPromise<void>;
-	$delete(handle: number, resource: UriComponents): TPromise<void>;
+	$delete(handle: number, resource: UriComponents, opts: FileDeleteOptions): TPromise<void>;
 	$watch(handle: number, session: number, resource: UriComponents, opts: IWatchOptions): void;
 	$unwatch(handle: number, session: number): void;
 }

diff --git a/src/vs/workbench/api/node/extHostFileSystem.ts b/src/vs/workbench/api/node/extHostFileSystem.ts
@@ -168,8 +168,8 @@ export class ExtHostFileSystem implements ExtHostFileSystemShape {
 		return asWinJsPromise(() => this._fsProvider.get(handle).writeFile(URI.revive(resource), Buffer.from(base64Content, 'base64'), opts));
 	}
 
-	$delete(handle: number, resource: UriComponents): TPromise<void, any> {
-		return asWinJsPromise(() => this._fsProvider.get(handle).delete(URI.revive(resource), { recursive: true }));
+	$delete(handle: number, resource: UriComponents, opts: files.FileDeleteOptions): TPromise<void, any> {
+		return asWinJsPromise(() => this._fsProvider.get(handle).delete(URI.revive(resource), opts));
 	}
 
 	$rename(handle: number, oldUri: UriComponents, newUri: UriComponents, opts: files.FileOverwriteOptions): TPromise<void, any> {

diff --git a/src/vs/workbench/parts/files/electron-browser/fileActions.ts b/src/vs/workbench/parts/files/electron-browser/fileActions.ts
@@ -661,7 +661,7 @@ class BaseDeleteFileAction extends BaseFileAction {
 					}
 
 					// Call function
-					const servicePromise = TPromise.join(distinctElements.map(e => this.fileService.del(e.resource, this.useTrash))).then(() => {
+					const servicePromise = TPromise.join(distinctElements.map(e => this.fileService.del(e.resource, { useTrash: this.useTrash, recursive: true }))).then(() => {
 						if (distinctElements[0].parent) {
 							this.tree.setFocus(distinctElements[0].parent); // move focus to parent
 						}

diff --git a/src/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.ts b/src/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.ts
@@ -339,8 +339,7 @@ export class BulkEdit {
 			if (edit.newUri && edit.oldUri) {
 				await this._textFileService.move(edit.oldUri, edit.newUri, overwrite);
 			} else if (!edit.newUri && edit.oldUri) {
-				// let recrusive = edit.options && edit.options.recursive;
-				await this._textFileService.delete(edit.oldUri, true);
+				await this._textFileService.delete(edit.oldUri, { useTrash: true, recursive: edit.options && edit.options.recursive });
 			} else if (edit.newUri && !edit.oldUri) {
 				let ignoreIfExists = edit.options && edit.options.ignoreIfExists;
 				if (!ignoreIfExists || !await this._fileService.existsFile(edit.newUri)) {

diff --git a/src/vs/workbench/services/files/electron-browser/fileService.ts b/src/vs/workbench/services/files/electron-browser/fileService.ts
@@ -899,7 +899,7 @@ export class FileService implements IFileService {
 					return TPromise.wrapError<boolean>(new Error(nls.localize('unableToMoveCopyError', "Unable to move/copy. File would replace folder it is contained in."))); // catch this corner case!
 				}
 
-				deleteTargetPromise = this.del(uri.file(targetPath));
+				deleteTargetPromise = this.del(uri.file(targetPath), { recursive: true });
 			}
 
 			return deleteTargetPromise.then(() => {
@@ -920,12 +920,12 @@ export class FileService implements IFileService {
 		});
 	}
 
-	public del(resource: uri, useTrash?: boolean): TPromise<void> {
-		if (useTrash) {
+	public del(resource: uri, options?: { useTrash?: boolean, recursive?: boolean }): TPromise<void> {
+		if (options && options.useTrash) {
 			return this.doMoveItemToTrash(resource);
 		}
 
-		return this.doDelete(resource);
+		return this.doDelete(resource, options && options.recursive);
 	}
 
 	private doMoveItemToTrash(resource: uri): TPromise<void> {
@@ -942,13 +942,34 @@ export class FileService implements IFileService {
 		return TPromise.as(void 0);
 	}
 
-	private doDelete(resource: uri): TPromise<void> {
+	private doDelete(resource: uri, recursive: boolean): TPromise<void> {
 		const absolutePath = this.toAbsolutePath(resource);
 
-		return pfs.del(absolutePath, os.tmpdir()).then(() => {
+		let assertNonRecursiveDelete: TPromise<void>;
+		if (!recursive) {
+			assertNonRecursiveDelete = pfs.stat(absolutePath).then(stat => {
+				if (!stat.isDirectory()) {
+					return TPromise.as(void 0);
+				}
+
+				return pfs.readdir(absolutePath).then(children => {
+					if (children.length === 0) {
+						return TPromise.as(void 0);
+					}
+
+					return TPromise.wrapError(new Error(nls.localize('deleteFailed', "Failed to delete non-empty folder '{0}'.", paths.basename(absolutePath))));
+				});
+			}, error => TPromise.as(void 0) /* ignore errors */);
+		} else {
+			assertNonRecursiveDelete = TPromise.as(void 0);
+		}
+
+		return assertNonRecursiveDelete.then(() => {
+			return pfs.del(absolutePath, os.tmpdir()).then(() => {
 
-			// Events
-			this._onAfterOperation.fire(new FileOperationEvent(resource, FileOperation.DELETE));
+				// Events
+				this._onAfterOperation.fire(new FileOperationEvent(resource, FileOperation.DELETE));
+			});
 		});
 	}
 

diff --git a/src/vs/workbench/services/files/electron-browser/remoteFileService.ts b/src/vs/workbench/services/files/electron-browser/remoteFileService.ts
@@ -519,12 +519,12 @@ export class RemoteFileService extends FileService {
 
 	// --- delete
 
-	del(resource: URI, useTrash?: boolean): TPromise<void> {
+	del(resource: URI, options?: { useTrash?: boolean, recursive?: boolean }): TPromise<void> {
 		if (resource.scheme === Schemas.file) {
-			return super.del(resource, useTrash);
+			return super.del(resource, options);
 		} else {
 			return this._withProvider(resource).then(RemoteFileService._throwIfFileSystemIsReadonly).then(provider => {
-				return provider.delete(resource).then(() => {
+				return provider.delete(resource, { recursive: options && options.recursive }).then(() => {
 					this._onAfterOperation.fire(new FileOperationEvent(resource, FileOperation.DELETE));
 				});
 			});
@@ -561,7 +561,7 @@ export class RemoteFileService extends FileService {
 	private _doMoveWithInScheme(source: URI, target: URI, overwrite?: boolean): TPromise<IFileStat> {
 
 		const prepare = overwrite
-			? this.del(target).then(undefined, err => { /*ignore*/ })
+			? this.del(target, { recursive: true }).then(undefined, err => { /*ignore*/ })
 			: TPromise.as(null);
 
 		return prepare.then(() => this._withProvider(source)).then(RemoteFileService._throwIfFileSystemIsReadonly).then(provider => {
@@ -582,7 +582,7 @@ export class RemoteFileService extends FileService {
 
 	private _doMoveAcrossScheme(source: URI, target: URI, overwrite?: boolean): TPromise<IFileStat> {
 		return this.copyFile(source, target, overwrite).then(() => {
-			return this.del(source);
+			return this.del(source, { recursive: true });
 		}).then(() => {
 			return this.resolveFile(target);
 		}).then(fileStat => {
@@ -615,7 +615,7 @@ export class RemoteFileService extends FileService {
 			}
 
 			const prepare = overwrite
-				? this.del(target).then(undefined, err => { /*ignore*/ })
+				? this.del(target, { recursive: true }).then(undefined, err => { /*ignore*/ })
 				: TPromise.as(null);
 
 			return prepare.then(() => {

diff --git a/src/vs/workbench/services/files/test/electron-browser/fileService.test.ts b/src/vs/workbench/services/files/test/electron-browser/fileService.test.ts
@@ -476,15 +476,15 @@ suite('FileService', () => {
 		});
 	});
 
-	test('deleteFolder', function () {
+	test('deleteFolder (recursive)', function () {
 		let event: FileOperationEvent;
 		const toDispose = service.onAfterOperation(e => {
 			event = e;
 		});
 
 		const resource = uri.file(path.join(testDir, 'deep'));
 		return service.resolveFile(resource).then(source => {
-			return service.del(source.resource).then(() => {
+			return service.del(source.resource, { recursive: true }).then(() => {
 				assert.equal(fs.existsSync(source.resource.fsPath), false);
 
 				assert.ok(event);
@@ -495,6 +495,17 @@ suite('FileService', () => {
 		});
 	});
 
+	test('deleteFolder (non recursive)', function () {
+		const resource = uri.file(path.join(testDir, 'deep'));
+		return service.resolveFile(resource).then(source => {
+			return service.del(source.resource).then(() => {
+				return TPromise.wrapError(new Error('Unexpected'));
+			}, error => {
+				return TPromise.as(true);
+			});
+		});
+	});
+
 	test('resolveFile', function () {
 		return service.resolveFile(uri.file(testDir), { resolveTo: [uri.file(path.join(testDir, 'deep'))] }).then(r => {
 			assert.equal(r.children.length, 8);

diff --git a/src/vs/workbench/services/textfile/common/textFileService.ts b/src/vs/workbench/services/textfile/common/textFileService.ts
@@ -707,10 +707,10 @@ export abstract class TextFileService implements ITextFileService {
 		});
 	}
 
-	public delete(resource: URI, useTrash?: boolean): TPromise<void> {
+	public delete(resource: URI, options?: { useTrash?: boolean, recursive?: boolean }): TPromise<void> {
 		const dirtyFiles = this.getDirty().filter(dirty => isEqualOrParent(dirty, resource, !platform.isLinux /* ignorecase */));
 
-		return this.revertAll(dirtyFiles, { soft: true }).then(() => this.fileService.del(resource, useTrash));
+		return this.revertAll(dirtyFiles, { soft: true }).then(() => this.fileService.del(resource, options));
 	}
 
 	public move(source: URI, target: URI, overwrite?: boolean): TPromise<void> {

diff --git a/src/vs/workbench/services/textfile/common/textfiles.ts b/src/vs/workbench/services/textfile/common/textfiles.ts
@@ -324,7 +324,7 @@ export interface ITextFileService extends IDisposable {
 	/**
 	 * Delete a file. If the file is dirty, it will get reverted and then deleted from disk.
 	 */
-	delete(resource: URI, useTrash?: boolean): TPromise<void>;
+	delete(resource: URI, options?: { useTrash?: boolean, recursive?: boolean }): TPromise<void>;
 
 	/**
 	 * Move a file. If the file is dirty, its contents will be preserved and restored.

diff --git a/src/vs/workbench/test/workbenchTestServices.ts b/src/vs/workbench/test/workbenchTestServices.ts
@@ -850,7 +850,7 @@ export class TestFileService implements IFileService {
 		return resource.scheme === 'file';
 	}
 
-	del(resource: URI, useTrash?: boolean): TPromise<void> {
+	del(resource: URI, options?: { useTrash?: boolean, recursive?: boolean }): TPromise<void> {
 		return TPromise.as(null);
 	}