Skip to content
This repository was archived by the owner on Nov 6, 2018. It is now read-only.

Support fetching references by provider name #45

Merged
merged 1 commit into from
Sep 9, 2018
Merged

Conversation

beyang
Copy link
Member

@beyang beyang commented Sep 6, 2018

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
Copy link

codecov bot commented Sep 6, 2018

Codecov Report

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

Impacted file tree graph

@@            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.

@beyang beyang force-pushed the bl/refs-group branch 5 times, most recently from c9dcf86 to 454a7e3 Compare September 6, 2018 05:19
@beyang beyang requested a review from sqs September 6, 2018 05:26
/**
* User-visible name of the of the provider being registered.
*/
providerName: string
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

registerOptions: { documentSelector },
registerOptions: {
documentSelector,
providerName: this.client.name,
Copy link
Member

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

@beyang beyang Sep 7, 2018

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

Could the providerName be added in the superclass

public register(message: RPCMessageType, data: RegistrationData<O>): void {
instead so that it doesn't have to be specified in each subclass?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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
Copy link
Member

sqs commented Sep 6, 2018

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
Copy link

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
Copy link
Member

sqs commented Sep 6, 2018

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
Copy link
Member Author

beyang commented Sep 7, 2018

@sqs @chrismwendt PTAL

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

@chrismwendt chrismwendt Sep 7, 2018

Choose a reason for hiding this comment

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

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

@beyang beyang force-pushed the bl/refs-group branch 2 times, most recently from f059338 to 1151c76 Compare September 7, 2018 22:56
@beyang beyang merged commit 322d416 into master Sep 9, 2018
@beyang beyang deleted the bl/refs-group branch September 9, 2018 03:02
@sourcegraph-bot
Copy link

🎉 This PR is included in version 14.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants