-
Notifications
You must be signed in to change notification settings - Fork 2
Support fetching references by provider name #45
Conversation
3a8afb6
to
7f235a4
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
c9dcf86
to
454a7e3
Compare
src/protocol/textDocument.ts
Outdated
/** | ||
* User-visible name of the of the provider being registered. | ||
*/ | ||
providerName: string |
There was a problem hiding this comment.
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
)
There was a problem hiding this comment.
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.
src/client/features/textDocument.ts
Outdated
registerOptions: { documentSelector }, | ||
registerOptions: { | ||
documentSelector, | ||
providerName: this.client.name, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
fromHoverMerged
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)
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
454a7e3
to
0f2d900
Compare
src/client/features/hover.ts
Outdated
registerOptions: { | ||
documentSelector, | ||
providerName: this.client.name, | ||
}, |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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?
src/client/features/textDocument.ts
Outdated
registerOptions: { documentSelector }, | ||
registerOptions: { | ||
documentSelector, | ||
providerName: this.client.name, |
There was a problem hiding this comment.
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
fromHoverMerged
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)
@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). |
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.
Yeah, this is at least asymmetric. More use cases will help us decide how to deal with this.
Agreed 👍 |
As for when it is useful, there actually are very many cases. Here are a couple (drawing from VS Code extensions):
The extensions could, of course, register a hover/code lens/etc. provider for |
@sqs @chrismwendt PTAL |
src/client/client.ts
Outdated
// from the same client having the same provider name. To avoid that, the server should supply | ||
// distinct provider names.) | ||
options.extensionID = this.id | ||
} |
There was a problem hiding this comment.
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"?
f059338
to
1151c76
Compare
1151c76
to
6601e94
Compare
🎉 This PR is included in version 14.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Adds
getLocationsWithProviderName
andprovidersWithName
toTextDocumentLocationProviderRegistry
. 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