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.

Sign up for GitHub

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

Jump to bottom

vscode.FileSystemProvider API feedback #48527

Closed
alexdima opened this issue Apr 24, 2018 · 4 comments
Closed

vscode.FileSystemProvider API feedback #48527

alexdima opened this issue Apr 24, 2018 · 4 comments
Assignees
@jrieken
Labels
remote Remote system operations issues verified Verification succeeded
Milestone
April 2018

Comments

@alexdima
Copy link
Member

alexdima commented Apr 24, 2018 •
edited by jrieken

Testing #48412

Grouping all of these suggestions in one item to make it easier to talk about them:

✅ 1. typo FileSystemProvider2 => FileSystemProvider

/**
 * An event to signal that a resource has been created, changed, or deleted. This
 * event should fire for resources that are being [watched](#FileSystemProvider2.watch)
 * by clients of this provider.
 */
readonly onDidChangeFile: Event<FileChangeEvent[]>;

✅ 2. arguments passed in (even via options) should perhaps not have optionals.

/**
 * Subscribe to events in the file or folder denoted by `uri`.
 * @param uri
 * @param options
 */
watch(uri: Uri, options: { recursive?: boolean; excludes?: string[] }): Disposable;
  1. Nail down as many stat options as possible now. Some ideas:
  • document that type information (via isFile, isDirectory or isSymbolicLink) is always mandatory
  • followSymlinks
  • includeSize
  • includeModifiedTime
  • includeCreationTime (see below)

✅ 4. Perhaps enlarge the stat already with creation time. What else are we missing from e.g. node's stat ?

  1. readDirectory should take in the exact same stat options as a sub-option. In fact, all of the methods that return a stat should take perhaps as a sub-option the options that can be passed into stat.

✅ 6. It is not clear if createDirectory should be recursive or not (for missing intermediate directories).
Same for writeFile.

✅ 7. It is not clear if delete should delete recursively.

✅ 8. Comment is inconsistent (remove "from the underlying storage" or add it everywhere...)

// Delete a file or folder from the underlying storage.

✅ 9. Parameter naming, use source and destination or something.

/**
 * Copy files or folders. Implementing this function is optional but it will speedup
 * the copy operation.
 *
 * @param uri The existing file or folder.
 * @param target The target location.
 * @param token A cancellation token.
 */
copy?(uri: Uri, target: Uri, options: FileOptions, token: CancellationToken): FileStat | Thenable<FileStat>;

✅ 10. What is a FileError.EntryNotFound error ?

/**
 * Retrieve metadata about a file. Throw an [`EntryNotFound`](#FileError.EntryNotFound)-error
 * in case the file does not exist.
 *
 * @param uri The uri of the file to retrieve meta data about.
 * @param token A cancellation token.
 * @return The file metadata about the file.
 */

✅ 11. Another typo FileType2:

/**
 * Retrieve the meta data of all entries of a [directory](#FileType2.Directory)
 *
 * @param uri The uri of the folder.
 * @param token A cancellation token.
 * @return A thenable that resolves to an array of tuples of file names and files stats.
 */
readDirectory(uri: Uri, options: { /*future: onlyType?*/ }, token: CancellationToken): [string, FileStat][] | Thenable<[string, FileStat][]>;
  1. Why does createDirectory return a stat and writeFile does not?

✅13. Should delete throw if the file or directory to be deleted is not found ?

✅14. Should rename throw if the source is not found or if the target exists? Should it overwrite the target ? Same question about copy.
@jrieken jrieken added this to the April 2018 milestone Apr 24, 2018
@jrieken jrieken added the remote Remote system operations issues label Apr 24, 2018
@dbaeumer
Copy link
Member

dbaeumer commented Apr 24, 2018 •
edited by jrieken

Will add my comments here as well:

  1. document inlined types
/**
 * Subscribe to events in the file or folder denoted by `uri`.
 * @param uri
 * @param options
 */
watch(uri: Uri, options: { recursive?: boolean; excludes?: string[] }): Disposable;

We should document recursicve and excludes

  1. Should we like NodeJS use functions instead of poperties for isFile, 'isDirectory`, .... (see https://nodejs.org/api/fs.html#fs_stats_isdirectory). Would it make more consistent and removes the discussion whether these properties are optional.

✅ 3. FileOptions have a read and write property and can be passed to readFile and writeFile. Do we need this at all. What happens if I call writeFile and have FileOptions.read = true.

  1. readDirectory in most cases we return literals instead of positional typed arrays. Do we win something instead of returning { name: string, stat: FileStat}[].

  2. writeFile do we need to communicate anything back in case the operation got canceled. Do we need to spec that if the operation got canceled either nothing got written or the whole content got written.

  3. same might be true for readFile. The result should not be incomplete in terms of bytes returned.

  4. delete do we need to communicate whether the delete was successful. Can fail due to various reasons. A common one for example is that the user has not the right permissions.

✅ 8. Do we need more FileSystemErrors. For example insufficient permissions.

  1. rename & copy should have a defined behavior when canceled as well.

jrieken added a commit that referenced this issue Apr 24, 2018
@jrieken
fix markdown-links in vscode.d.ts, #48527
d2944d7
@dbaeumer dbaeumer mentioned this issue Apr 24, 2018
Closed
2 tasks
jrieken added a commit that referenced this issue Apr 24, 2018
@jrieken
argument names, #48527
36c5c31
jrieken added a commit that referenced this issue Apr 25, 2018
@jrieken
jsdoc-polish, #48527
3907d47
jrieken added a commit that referenced this issue Apr 25, 2018
@jrieken
no optional fields in watch-options, #48527
aa63600
jrieken added a commit that referenced this issue Apr 25, 2018
@jrieken
have a FileType and make readDirectory slimmer, #48527
fb590e6
jrieken added a commit that referenced this issue Apr 26, 2018
@jrieken
remove CancelationTokens, #48527
96a1994
jrieken added a commit that referenced this issue Apr 26, 2018
@jrieken
remove unused/underspecified options, #48527
923bbd4
jrieken added a commit that referenced this issue Apr 26, 2018
@jrieken
add NoPermissions-error, more jsdoc, #48527
6cbbb7e
@jrieken
Copy link
Member

jrieken commented Apr 26, 2018

Some general change notice: Based on @dbaeumer's feedback and discussion I have removed cancelation-tokens (we can still add them later). Based @alexandrudima feedback I have removed most kitchen-sink-who-knows-what-comes-options because we can only ever use them to relax an operation. Not having tokens (that block the last argument-position) makes it nicer to eventually add them back.

jrieken added a commit that referenced this issue Apr 26, 2018
@jrieken
no implicit stat objects, #48527
2ed98bf
jrieken added a commit that referenced this issue Apr 26, 2018
@jrieken
fix mkdirp and use it more, #48527
84238e7
jrieken added a commit that referenced this issue Apr 26, 2018
@jrieken
jsdoc fixes, #48527
72b51f3
@jrieken
Copy link
Member

jrieken commented Apr 26, 2018

Why does createDirectory return a stat and writeFile does not?

Fixed, now only stat returns FileStat and other operations return nothing.

@jrieken jrieken closed this as completed Apr 26, 2018
@dbaeumer
Copy link
Member

Marking as verified. Joh and I went over the API yesterday again in a review session.

@dbaeumer dbaeumer added the verified Verification succeeded label Apr 27, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Jun 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jrieken jrieken

Labels
remote Remote system operations issues verified Verification succeeded
Projects
None yet
Milestone
April 2018
Development

No branches or pull requests
3 participants
@jrieken @dbaeumer @alexdima