sourcegraph / sourcegraph-extension-api

Sourcegraph extension API: use and build extensions that enhance reading and reviewing code in your existing tools. "The extension API you wish your code host had."
44 stars 2 forks source link

Support fetching references by provider name #45

Closed beyang closed 6 years ago

beyang commented 6 years ago

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[bot] commented 6 years ago

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% <ø> (ø) :arrow_up:
src/client/features/common.ts 85.29% <100%> (+0.44%) :arrow_up:
src/client/client.ts 94.28% <100%> (+0.05%) :arrow_up:
src/environment/providers/registry.ts 94.44% <100%> (ø) :arrow_up:
src/client/features/decoration.ts 63.33% <75%> (+1.79%) :arrow_up:
src/environment/providers/location.ts 81.08% <76.92%> (-2.26%) :arrow_down:
src/client/features/location.ts 94.28% <80%> (-2.39%) :arrow_down:
src/client/features/hover.ts 90% <80%> (-3.34%) :arrow_down:

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.

sqs commented 6 years ago

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 commented 6 years ago

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 :+1:

sqs commented 6 years ago

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):

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 commented 6 years ago

@sqs @chrismwendt PTAL

sourcegraph-bot commented 6 years ago

:tada: This PR is included in version 14.1.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: