WIP: workspace file system API #120

sqs · 2018-11-02T07:13:51Z

I made an attempt at an extension API for reading dirs and files from a repository.

The code is WIP, so don't bother reviewing it for implementation feedback. But API feedback would be helpful.

felixfbecker

I was to design/name this API I would avoid the concept of a "file system" and not use syscall-like names, because that seems like an implementation detail (and doesn't actually match the implementation, since it is backed by GraphQL API and git show/git ls-files). As a user, I don't care about "reading a file" I care about "getting the content".
I said the same back in the day about "vfs" in LSP :)

felixfbecker · 2018-11-02T10:44:59Z

src/sourcegraph.d.ts

+         *
+         * @todo Document what happens when the root is a subdirectory of a repository.
+         */
+        readonly repository?: {


What if the root has multiple repos?

If the root is itself not in a repository, then this would be undefined (even if it had subdirs that were repositories). This only describes "where the workspace root comes from, from the POV of the client application". It is not a general "all repository information that might be relevant for this root" because that:

can change at any time

is difficult to define minimally

chrismwendt

The API looks good to me! It might be useful to expose a readDirectoryRecursive function at some point, but no need for that now.

chrismwendt · 2018-11-02T21:34:52Z

src/sourcegraph.d.ts

+             *
+             * This is equivalent to Repository.id in the Sourcegraph GraphQL API schema.
+             */
+            readonly id: string


Just wondering, what would the id here be used for? Perhaps renaming a repository via the Sourcegraph GraphQL API? Where does an extension get an id from?

If an extension needs a stable reference identifier for the repository, it can use this value. Handling renames/deletions for any operation is one case where this is useful. In general, if the extension stores other data that is attached to the repository itself and not just the name, then it should use this identifier. Other cases of that are:

long-running operations

caches

external services that store a link or permission entry pointing to a Sourcegraph repository

codecov · 2018-11-03T06:48:18Z

Codecov Report

Merging #120 into master will decrease coverage by 76.41%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #120       +/-   ##
==========================================
- Coverage   77.62%   1.21%   -76.42%     
==========================================
  Files          67      71        +4     
  Lines        2632    2808      +176     
  Branches      534     558       +24     
==========================================
- Hits         2043      34     -2009     
- Misses        589    2774     +2185

Impacted Files	Coverage Δ
src/protocol/plainTypes.ts	`0% <ø> (ø)`	⬆️
src/client/environment.ts	`100% <ø> (ø)`	⬆️
src/client/api/roots.ts	`0% <0%> (ø)`
src/extension/api/roots.ts	`0% <0%> (ø)`
src/shared/uri.ts	`0% <0%> (ø)`
src/extension/workerMain.ts	`0% <0%> (ø)`	⬆️
src/extension/types/location.ts	`0% <0%> (-65%)`	⬇️
src/client/controller.ts	`0% <0%> (-86.45%)`	⬇️
src/extension/extensionHost.ts	`0% <0%> (-87.5%)`	⬇️
src/extension/api/fileSystem.ts	`0% <0%> (ø)`
... and 64 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7a6d85...37aea1c. Read the comment docs.

sqs · 2018-11-03T08:16:07Z

@felixfbecker:

I was to design/name this API I would avoid the concept of a "file system" and not use syscall-like names, because that seems like an implementation detail (and doesn't actually match the implementation, since it is backed by GraphQL API and git show/git ls-files)

Are you thinking of changes other than the names? What specific changes do you have in mind?

(This implementation is backed by GraphQL, but other implementations, such as in the editor or when the extension is itself a file system provider (e.g., for vendored or codegenned files) would be more appropriately called "file systems". So I think the name fits, but I am interested to hear your alternatives.)

sqs · 2018-11-03T08:18:05Z

The API looks good to me! It might be useful to expose a readDirectoryRecursive function at some point, but no need for that now.

I added a TODO in the sourcegraph.d.ts file about exposing a way to get all files matching a glob path.

The enhanced sourcegraph.URI class (and its implementation, which is shared by the client and extension) is a full URI parser. Previously it only wrapped a string.

chrismwendt · 2018-11-05T03:50:45Z

Here's how to move this PR to https://github.com/sourcegraph/sourcegraph/tree/master/packages/sourcegraph-extension-api

cd browser-extensions
git format-patch master --stdout > /tmp/patch

cd ../sourcegraph
cat /tmp/patch | git am -3 --directory=packages/sourcegraph-extension-api/
# and fixup merge conflicts

felixfbecker · 2018-11-05T23:48:32Z

src/shared/uri.ts

+ *
+ *   new URL("https://foo/bar").pathname === "/bar"
+ *
+ *   new URL("git://foo/bar").pathname === "//foo/bar" // UNDESIRABLE


This is not how WHATWG URL behaves: https://runkit.com/felixfbecker/5be0d65fa5064900123995f4

Then I think it's not spec compliant (and so soon will change, or will have different behavior in different browsers). Try it in the browser and read the https://url.spec.whatwg.org/#special-scheme doc.

sqs requested review from chrismwendt and felixfbecker November 2, 2018 07:13

felixfbecker reviewed Nov 2, 2018

View reviewed changes

chrismwendt approved these changes Nov 2, 2018

View reviewed changes

sqs force-pushed the files branch from 0ce6baf to a8c30aa Compare November 3, 2018 08:26

sqs added 5 commits November 3, 2018 03:43

chore(deps): upgrade @types/node

5cf13e4

refactor: move class URI to shared

18ab139

feat: parse and expose URIs in extension API

f5b456e

The enhanced sourcegraph.URI class (and its implementation, which is shared by the client and extension) is a full URI parser. Previously it only wrapped a string.

feat: expose workspace roots and file system API

0eb7b95

fix: wrap extensionHostWorkerMain in try..catch

37aea1c

sqs force-pushed the files branch from 227a355 to 37aea1c Compare November 3, 2018 10:52

felixfbecker reviewed Nov 5, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: workspace file system API #120

WIP: workspace file system API #120

sqs commented Nov 2, 2018

felixfbecker left a comment

felixfbecker Nov 2, 2018

sqs Nov 3, 2018

chrismwendt left a comment

chrismwendt Nov 2, 2018

sqs Nov 3, 2018

codecov bot commented Nov 3, 2018 •

edited

Loading

sqs commented Nov 3, 2018

sqs commented Nov 3, 2018

chrismwendt commented Nov 5, 2018

felixfbecker Nov 5, 2018

sqs Nov 6, 2018 •

edited

Loading

WIP: workspace file system API #120

Are you sure you want to change the base?

WIP: workspace file system API #120

Conversation

sqs commented Nov 2, 2018

felixfbecker left a comment

Choose a reason for hiding this comment

felixfbecker Nov 2, 2018

Choose a reason for hiding this comment

sqs Nov 3, 2018

Choose a reason for hiding this comment

chrismwendt left a comment

Choose a reason for hiding this comment

chrismwendt Nov 2, 2018

Choose a reason for hiding this comment

sqs Nov 3, 2018

Choose a reason for hiding this comment

codecov bot commented Nov 3, 2018 • edited Loading

Codecov Report

sqs commented Nov 3, 2018

sqs commented Nov 3, 2018

chrismwendt commented Nov 5, 2018

felixfbecker Nov 5, 2018

Choose a reason for hiding this comment

sqs Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 3, 2018 •

edited

Loading

sqs Nov 6, 2018 •

edited

Loading