Support fetching references by provider name #45

beyang · 2018-09-06T04:08:30Z

Adds getLocationsWithProviderName and providersWithName to TextDocumentLocationProviderRegistry. The former returns locations with their associated provider's name and the latter returns a list of the providers with their name.

This should be reviewed in conjunction with https://github.com/sourcegraph/sourcegraph/pull/13126

codecov · 2018-09-06T04:14:32Z

Codecov Report

Merging #45 into master will increase coverage by 0.01%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
+ Coverage   78.65%   78.67%   +0.01%     
==========================================
  Files          74       74              
  Lines        2858     2888      +30     
  Branches      552      557       +5     
==========================================
+ Hits         2248     2272      +24     
- Misses        610      616       +6

Impacted Files	Coverage Δ
src/protocol/textDocument.ts	`100% <ø> (ø)`	⬆️
src/client/features/common.ts	`85.29% <100%> (+0.44%)`	⬆️
src/client/client.ts	`94.28% <100%> (+0.05%)`	⬆️
src/environment/providers/registry.ts	`94.44% <100%> (ø)`	⬆️
src/client/features/decoration.ts	`63.33% <75%> (+1.79%)`	⬆️
src/environment/providers/location.ts	`81.08% <76.92%> (-2.26%)`	⬇️
src/client/features/location.ts	`94.28% <80%> (-2.39%)`	⬇️
src/client/features/hover.ts	`90% <80%> (-3.34%)`	⬇️

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 163a8ee...6601e94. Read the comment docs.

src/client/client.ts

sqs · 2018-09-06T05:31:58Z

src/protocol/textDocument.ts

+    /**
+     * User-visible name of the of the provider being registered.
+     */
+    providerName: string


Based on how you're using it elsewhere, this should be optional (ie providerName?: string)

Nope, it should be required. Otherwise, it will show up as blank in the provider selector. Added a check to guarantee this where the type is asserted.

src/environment/providers/util.ts

sqs · 2018-09-06T05:34:53Z

src/client/features/textDocument.ts

-                registerOptions: { documentSelector },
+                registerOptions: {
+                    documentSelector,
+                    providerName: this.client.name,


This is mixing the concepts of a client name (which is always set to the extension ID) and the provider name (there can be multiple providers per extension). This is fine for simplicity for now, but if you choose to keep these conflated, document that it is a temporary simplification.

Since the client ID is synonymous with extension ID, and the client name was completely unused prior to this PR, maybe these concepts (client ID, client name, extension ID, provider name) could be unified under extension ID, which means we don't need this new provider name concept 🤔

@sqs I don't see multiple providers per extension causing ambiguity because the client always asks the controller for results for one capability at a time (e.g. getHover) and each extension either provides or does not provide the capability - that is, no extension will provide a capability more than once. Please correct me if I'm not understanding a relevant part of this 🙇

More concretely, we could:

Get rid of client name (unused)

Do not introduce provider name in this PR

Unify client ID and extension ID (just call it extension ID)

Change the return type of methods such as getHover from HoverMerged to { extension: ID, response: Hover }[] (essentially tagging responses with the extension ID from which they came, and this could be factored out as a type function for convenience), and transfer responsibility for merging results to the clients such as the web app (this is a half baked idea, might not work)

I ended up removing Client.name, since it was identical to Client.id in all instances. That okay?

src/environment/providers/location.ts

sqs · 2018-09-06T05:37:49Z

src/environment/providers/location.ts

+        registrationOptions: TextDocumentRegistrationOptions,
+        provider: ProvideTextDocumentLocationSignature<P, L>
+    ): Unsubscribable {
+        if (!registrationOptions.providerName) {


It would be nice if there were a way to enforce this with the type system somewhere at a higher level than in this method body. Right now, the way you have it, you have some values of type TextDocumentRegistrationOptions that have a null or undefined providerName property. That means the type is lying, which is generally not good. Ideally you can make it so that the validation and cast occurs in the same place and the value never has a type that is incorrect.

I added the abstract validateRegistrationOptions method to Feature and added implementations for all concrete subclasses. This is necessary, because we need to assert the validity of the options object when deserializing the registration request. This seemed like a reasonably clean way to do it that keeps the validating logic in the class that needs the type safety.

src/environment/providers/location.ts

chrismwendt · 2018-09-06T18:04:36Z

src/client/features/hover.ts

+            registerOptions: {
+                documentSelector,
+                providerName: this.client.name,
+            },


Could the providerName be added in the superclass

sourcegraph-extension-api/src/client/features/common.ts

Line 66 in 454a7e3

public register(message: RPCMessageType, data: RegistrationData<O>): void {

instead so that it doesn't have to be specified in each subclass?

I'm not sure I understand. providerName (now renamed extensionID) is a field in an interface, not a class. What's the superclass here?

chrismwendt · 2018-09-06T19:34:40Z

src/client/features/textDocument.ts

-                registerOptions: { documentSelector },
+                registerOptions: {
+                    documentSelector,
+                    providerName: this.client.name,


Since the client ID is synonymous with extension ID, and the client name was completely unused prior to this PR, maybe these concepts (client ID, client name, extension ID, provider name) could be unified under extension ID, which means we don't need this new provider name concept 🤔

@sqs I don't see multiple providers per extension causing ambiguity because the client always asks the controller for results for one capability at a time (e.g. getHover) and each extension either provides or does not provide the capability - that is, no extension will provide a capability more than once. Please correct me if I'm not understanding a relevant part of this 🙇

More concretely, we could:

Get rid of client name (unused)

Do not introduce provider name in this PR

Unify client ID and extension ID (just call it extension ID)

Change the return type of methods such as getHover from HoverMerged to { extension: ID, response: Hover }[] (essentially tagging responses with the extension ID from which they came, and this could be factored out as a type function for convenience), and transfer responsibility for merging results to the clients such as the web app (this is a half baked idea, might not work)

sqs · 2018-09-06T19:55:20Z

I don't see multiple providers per extension causing ambiguity because the client always asks the controller for results for one capability at a time (e.g. getHover) and each extension either provides or does not provide the capability - that is, no extension will provide a capability more than once.

@chrismwendt Hmm, good point. It is still possible to registerCapability 2x from the same extension. When the actual textDocument/hovers are sent, the JSON-RPC message params doesn't contain an indication of which hover provider is being called, but the client does know that. So this is like a weird mismatch of our concepts on the client and server sides.

So I think @beyang should address the direct feedback and not worry about the ideal design here. As long as this doesn't change the RPC protocol (which it doesn't), it is easy for us to change this API later (such as today when we type up the ideal api.ts definition).

chrismwendt · 2018-09-06T21:54:07Z

It is still possible to registerCapability 2x from the same extension.

This sounds like some kind of protocol violation to me and I'm not aware of a case where this would be useful (are you?). The property that only one provider can be registered for a given capability at any point in time can be upheld by ignoring extra registrations.

When the actual textDocument/hovers are sent, the JSON-RPC message params doesn't contain an indication of which hover provider is being called, but the client does know that. So this is like a weird mismatch of our concepts on the client and server sides.

Yeah, this is at least asymmetric. More use cases will help us decide how to deal with this.

So I think @beyang should address the direct feedback and not worry about the ideal design here.

Agreed 👍

sqs · 2018-09-06T22:01:50Z

This sounds like some kind of protocol violation to me and I'm not aware of a case where this would be useful (are you?). The property that only one provider can be registered for a given capability at any point in time can be upheld by ignoring extra registrations.

RegistrationData#id allows for multiple registrations for the same thing to be distinguished from each other.

As for when it is useful, there actually are very many cases. Here are a couple (drawing from VS Code extensions):

GitLens registers multiple hover providers: one to show hovers over the end-of-line blame and one to show hovers for the beginning-of-line blame
TypeScript registers multiple code lens providers: one to show references code lens, one to show implementations code lens

The extensions could, of course, register a hover/code lens/etc. provider for * and multiplex it inside the extension, but VS Code chose to let extensions register multiple providers with their own specific documentSelectors (and not need to implement multiplexing in the extension). That seems like a good decision for extension authors.

beyang · 2018-09-07T04:24:00Z

@sqs @chrismwendt PTAL

chrismwendt · 2018-09-07T05:00:21Z

src/client/client.ts

+                // from the same client having the same provider name. To avoid that, the server should supply
+                // distinct provider names.)
+                options.extensionID = this.id
+            }


Should the comment say "from the same extension" rather than "from the same client"?

sourcegraph-bot · 2018-09-09T03:05:25Z

🎉 This PR is included in version 14.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

beyang force-pushed the bl/refs-group branch from 3a8afb6 to 7f235a4 Compare September 6, 2018 04:14

beyang force-pushed the bl/refs-group branch 5 times, most recently from c9dcf86 to 454a7e3 Compare September 6, 2018 05:19

beyang requested a review from sqs September 6, 2018 05:26

sqs reviewed Sep 6, 2018

View reviewed changes

beyang force-pushed the bl/refs-group branch from 454a7e3 to 0f2d900 Compare September 6, 2018 18:23

chrismwendt reviewed Sep 6, 2018

View reviewed changes

sqs approved these changes Sep 7, 2018

View reviewed changes

chrismwendt approved these changes Sep 7, 2018

View reviewed changes

beyang force-pushed the bl/refs-group branch 2 times, most recently from f059338 to 1151c76 Compare September 7, 2018 22:56

feat: add method to get locations with extension ID

6601e94

beyang force-pushed the bl/refs-group branch from 1151c76 to 6601e94 Compare September 9, 2018 02:28

beyang merged commit 322d416 into master Sep 9, 2018

beyang deleted the bl/refs-group branch September 9, 2018 03:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support fetching references by provider name #45

Support fetching references by provider name #45

beyang commented Sep 6, 2018 •

edited

Loading

codecov bot commented Sep 6, 2018 •

edited

Loading

sqs Sep 6, 2018

beyang Sep 7, 2018

sqs Sep 6, 2018

chrismwendt Sep 6, 2018

beyang Sep 7, 2018 •

edited

Loading

sqs Sep 6, 2018

beyang Sep 7, 2018

chrismwendt Sep 6, 2018

beyang Sep 7, 2018

chrismwendt Sep 6, 2018

sqs commented Sep 6, 2018

chrismwendt commented Sep 6, 2018

sqs commented Sep 6, 2018

beyang commented Sep 7, 2018

chrismwendt Sep 7, 2018 •

edited

Loading

sourcegraph-bot commented Sep 9, 2018

Support fetching references by provider name #45

Support fetching references by provider name #45

Conversation

beyang commented Sep 6, 2018 • edited Loading

codecov bot commented Sep 6, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beyang Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sqs commented Sep 6, 2018

chrismwendt commented Sep 6, 2018

sqs commented Sep 6, 2018

beyang commented Sep 7, 2018

chrismwendt Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

sourcegraph-bot commented Sep 9, 2018

beyang commented Sep 6, 2018 •

edited

Loading

codecov bot commented Sep 6, 2018 •

edited

Loading

beyang Sep 7, 2018 •

edited

Loading

chrismwendt Sep 7, 2018 •

edited

Loading