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

Add documentation for FileService & FileSystemProvider #8596

Merged
merged 1 commit into from
Oct 13, 2020

Conversation

tortmayr
Copy link
Contributor

@tortmayr tortmayr commented Oct 7, 2020

Fixes #8599
Contributed on behalf of STMicroelectronics
Signed-off-by: Tobias Ortmayr tortmayr@eclipsesource.com

What it does

Add documentation for core components of the FileService & FileSystemProvider API

How to test

Documentation changes only

Review checklist

Reminder for reviewers

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tortmayr thank you for your contribution, please resolve the build errors before we can proceed with a review:

@vince-fugnitto vince-fugnitto added the documentation issues related to documentation label Oct 7, 2020
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of typos, I haven't read through the actual documentation yet.

packages/filesystem/src/common/files.ts Outdated Show resolved Hide resolved
packages/filesystem/src/common/files.ts Outdated Show resolved Hide resolved
packages/filesystem/src/common/files.ts Outdated Show resolved Hide resolved
packages/filesystem/src/common/files.ts Outdated Show resolved Hide resolved
packages/filesystem/src/common/files.ts Outdated Show resolved Hide resolved
packages/filesystem/src/browser/file-service.ts Outdated Show resolved Hide resolved
packages/filesystem/src/browser/file-service.ts Outdated Show resolved Hide resolved
packages/filesystem/src/browser/file-service.ts Outdated Show resolved Hide resolved
packages/filesystem/src/browser/file-service.ts Outdated Show resolved Hide resolved
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tortmayr please be sure to address the build issues (I believe formatting errors), and squash the commits into a single commit with the sign-off. There's no need to co-author me :) all I did was provide a review.

@sdirix
Copy link
Member

sdirix commented Oct 9, 2020

Hi @vince-fugnitto thanks for the review! I addressed the remaining issue and squashed the commits.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the content of the documentation and it looks very good!
I identified a few minor comments (formatting, typos) to address.

* @param resource `URI` of the resource to test.
* @param capability The required capability.
*
* @returns `true` if the resource can be handled and the required cabability can be provided.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @returns `true` if the resource can be handled and the required cabability can be provided.
* @returns `true` if the resource can be handled and the required capability can be provided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

* @param to `URI` of the target location.
* @param opts Options to define if existing files should be overwritten.
*/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there is an incorrectly added extra line.


/**
* Optional function that has to be implemented by {@link FileSystemProviderWithFileFolderCopyCapability}.
* see {@link FileSystemProviderWithFileFolderCopyCapability#copy}} for additional documentation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are extra whitespaces after @link for the following methods.
Please ignore if these are done on purpose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure that's a mistake ;)

fixes eclipse-theia#8599

Contributed on behalf of STMicroelectronics
Signed-off-by: Tobias Ortmayr <tortmayr@eclipsesource.com>
Signed-off-by: Stefan Dirix <sdirix@eclipsesource.com>
@sdirix
Copy link
Member

sdirix commented Oct 9, 2020

Addressed your comments and added some minor additional changes, adjusting whitespace and upper cases in beginning of sentences.

Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking care of Theia ❤️

@kittaakos kittaakos merged commit 40b3800 into eclipse-theia:master Oct 13, 2020
* Retrieve the content of a given directory.
* @param resource The `URI` of the directory.
*
* @returns A map containing the {@link FileType} for each child resource (uri).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tortmayr, @sdirix isn't it the name of the resource instead of the URI? Can you please check? Thank you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check and come back to you 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! VS Code uses name. I implemented an FSProvider based on your documentation yesterday, right before I merged it, and readdir did not work.

Copy link
Member

@sdirix sdirix Oct 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you're correct. Debugging into a standard Theia application the existing providers only return file names here, both in browser and in node.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for checking it. Do you happen to have time to prepare a follow-up PR with the correction?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll open one in a short time 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR is here: #8638.

Thanks for noticing this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation issues related to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Documentation for FileService API
4 participants