-
Notifications
You must be signed in to change notification settings - Fork 2
Add search features API #95
Changes from 5 commits
efa285d
5e4fe5b
de15667
f5fd2be
e10a9fc
25ca64b
c76d18c
8a1c7aa
39590b9
4847f2b
13e53ba
4bbef15
c112fa7
61756bc
f1326e3
480071b
4c57cae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
import { from, Observable, Subscription } from 'rxjs' | ||
import { ExtSearchFeaturesAPI } from 'src/extension/api/search' | ||
import { Connection } from 'src/protocol/jsonrpc2/connection' | ||
import { createProxyAndHandleRequests } from '../../common/proxy' | ||
import { FeatureProviderRegistry } from '../providers/registry' | ||
import { TransformQuerySignature } from '../providers/search' | ||
import { SubscriptionMap } from './common' | ||
|
||
export interface SearchFeaturesAPI { | ||
$registerSearchProvider(id: number): void | ||
$unregister(id: number): void | ||
} | ||
|
||
export class SearchFeatures implements SearchFeaturesAPI { | ||
private subscriptions = new Subscription() | ||
private registrations = new SubscriptionMap() | ||
private proxy: ExtSearchFeaturesAPI | ||
|
||
constructor(connection: Connection, private searchRegistry: FeatureProviderRegistry<{}, TransformQuerySignature>) { | ||
this.subscriptions.add(this.registrations) | ||
|
||
this.proxy = createProxyAndHandleRequests('searchFeatures', connection, this) | ||
} | ||
|
||
public $registerSearchProvider(id: number): void { | ||
this.registrations.add( | ||
id, | ||
this.searchRegistry.registerProvider( | ||
{}, | ||
(query: string): Observable<string | null | undefined> => from(this.proxy.$transformQuery(id, query)) | ||
) | ||
) | ||
} | ||
|
||
public $unregister(id: number): void { | ||
this.registrations.remove(id) | ||
} | ||
|
||
public unsubscribe(): void { | ||
this.subscriptions.unsubscribe() | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
import { Observable } from 'rxjs' | ||
import { switchMap } from 'rxjs/operators' | ||
import { FeatureProviderRegistry } from './registry' | ||
|
||
export type TransformQuerySignature = (query: string) => Observable<string | null | undefined> | ||
|
||
/** Provides transformed queries from the first search extension. */ | ||
export class SearchTransformProviderRegistry extends FeatureProviderRegistry<{}, TransformQuerySignature> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it a search transform, a query transform, or what? Be consistent. I think that "query transformer" is the best term to use. So |
||
public transformQuery(query: string): Observable<string | null | undefined> { | ||
return transformQuery(this.providers, query) | ||
} | ||
} | ||
|
||
/** | ||
* Returns an observable that emits the first provider's transformed query whenever any of the last-emitted set of providers emits | ||
* a query. | ||
* | ||
* Most callers should use SearchTransformProviderRegistry's getHover method, which uses the registered search | ||
* providers. | ||
*/ | ||
export function transformQuery( | ||
providers: Observable<TransformQuerySignature[]>, | ||
query: string | ||
): Observable<string | null | undefined> { | ||
return providers.pipe( | ||
switchMap(providers => { | ||
if (providers.length === 0) { | ||
return [query] | ||
} | ||
return providers[0](query) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could these be chained? Something like |
||
}) | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
import { Unsubscribable } from 'rxjs' | ||
import { SearchProvider } from 'sourcegraph' | ||
import { SearchFeaturesAPI } from 'src/client/api/search' | ||
import { ProviderMap } from './common' | ||
|
||
/** @internal */ | ||
export interface ExtSearchFeaturesAPI { | ||
$transformQuery: (id: number, query: string) => Promise<string | null | undefined> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned above, I think you should only accept |
||
} | ||
|
||
/** @internal */ | ||
export class ExtSearchFeatures implements ExtSearchFeaturesAPI { | ||
private registrations = new ProviderMap<SearchProvider>(id => this.proxy.$unregister(id)) | ||
constructor(private proxy: SearchFeaturesAPI) {} | ||
|
||
public registerSearchProvider(provider: SearchProvider): Unsubscribable { | ||
const { id, subscription } = this.registrations.add(provider) | ||
this.proxy.$registerSearchProvider(id) | ||
return subscription | ||
} | ||
|
||
public $transformQuery(id: number, query: string): Promise<string | null | undefined> { | ||
const provider = this.registrations.get<SearchProvider>(id) | ||
return Promise.resolve(provider.transformQuery(query)) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -717,6 +717,14 @@ declare module 'sourcegraph' { | |
): ProviderResult<Location[]> | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you should just call it
|
||
export interface SearchProvider { | ||
transformQuery(query: string): ProviderResult<string> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wrote 2 comments above about:
I see why you included null and undefined now, because The reason why Another related reason you should use
There is no need for the 2nd and 3rd ways of expressing this. |
||
} | ||
|
||
export namespace search { | ||
export function registerSearchProvider(provider: SearchProvider): Unsubscribable | ||
} | ||
|
||
export namespace languages { | ||
export function registerHoverProvider(selector: DocumentSelector, provider: HoverProvider): Unsubscribable | ||
|
||
|
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.
Can just call it Search, not SearchFeatures. LanguageFeatures is called that because just "Language" alone would be confusing. That problem does not exist for search. (Update all instances of "searchFeatures" too.)