Add search features API #95

attfarhan · 2018-10-24T00:51:43Z

This adds the first part of the new search features API. It currently allows extensions to transform user queries.

codecov · 2018-10-24T00:54:17Z

Codecov Report

Merging #95 into master will increase coverage by 0.33%.
The diff coverage is 95.83%.

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
+ Coverage   78.07%   78.41%   +0.33%     
==========================================
  Files          61       64       +3     
  Lines        2472     2520      +48     
  Branches      521      522       +1     
==========================================
+ Hits         1930     1976      +46     
- Misses        542      544       +2

Impacted Files	Coverage Δ
src/client/providers/queryTransformer.ts	`100% <100%> (ø)`
src/client/registries.ts	`100% <100%> (ø)`	⬆️
src/extension/api/search.ts	`100% <100%> (ø)`
src/extension/extensionHost.ts	`88.23% <100%> (+0.73%)`	⬆️
src/client/controller.ts	`86.2% <100%> (+0.24%)`	⬆️
src/client/api/search.ts	`87.5% <87.5%> (ø)`

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 fd0d000...4c57cae. Read the comment docs.

chrismwendt

Can you provide a small example of what this looks like when used in a Sourcegraph extension?

Make sure the naming here won't clash if/when we add providers for search results like JIRA or GitHub issues.

chrismwendt · 2018-10-24T01:23:01Z

src/client/providers/search.ts

+            if (providers.length === 0) {
+                return [query]
+            }
+            return providers[0](query)


Could these be chained? Something like provider[0](query).then(q1 => provider[1](q1)).then(...)?

attfarhan · 2018-10-24T16:53:36Z

@chrismwendt here's an example extension: https://github.com/attfarhan/search-extension

Perhaps we could change the naming from SearchProvider to QueryTransformProvider or something similar to be more descriptive/accurate. WDYT?

chrismwendt · 2018-10-24T18:03:40Z

QueryTransformProvider sounds good 👍

sqs

Add tests for all newly added code. There are existing tests for similar things to everything you've done, so you can start with those.

sqs · 2018-10-24T19:22:32Z

src/client/api/search.ts

+    $unregister(id: number): void
+}
+
+export class SearchFeatures implements SearchFeaturesAPI {


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

sqs · 2018-10-24T19:24:19Z

src/client/providers/search.ts

+export type TransformQuerySignature = (query: string) => Observable<string | null | undefined>
+
+/** Provides transformed queries from the first search extension. */
+export class SearchTransformProviderRegistry extends FeatureProviderRegistry<{}, TransformQuerySignature> {


Is it a search transform, a query transform, or what? Be consistent.

I think that "query transformer" is the best term to use. So QueryTransformerRegistry, registerQueryTransformer, etc.

attfarhan · 2018-10-25T06:47:38Z

@chrismwendt @sqs PTAL

sqs

This looks fantastic! Nice job. I added a lot of comments about very picky details.

sqs · 2018-10-26T05:08:12Z

src/client/api/search.ts

+import { FeatureProviderRegistry } from '../providers/registry'
+import { SubscriptionMap } from './common'
+
+export interface SearchAPI {


Add /** @internal */ tsdoc directives as in https://sourcegraph.com/github.com/sourcegraph/sourcegraph-extension-api/-/blob/src/client/api/configuration.ts#L8:1 and other files because this type is part of the internal API (so we want to communicate that to readers and to tools like doc generators).

sqs · 2018-10-26T05:10:48Z

src/client/providers/queryTransformer.test.ts

+                expectObservable(
+                    transformQuery(
+                        cold<TransformQuerySignature[]>('-a-b-|', {
+                            a: [q => of(q)],


If the first query transform is just the identity func, then this doesn't really test the behavior of "all query transform funcs are applied". Instead you should test something like:

1st query transform func appends a to the string

2nd query transform func appends b to the string

and test that the result is the original string with ab appended.

(I think the current test would pass even if your logic was like this.providers[1](query), which would obviously be incorrect and would not be caught by the test.)

sqs · 2018-10-26T05:12:20Z

src/client/providers/queryTransformer.ts

+
+export type TransformQuerySignature = (query: string) => Observable<string | null | undefined>
+
+/** Provides transformed queries from the first search extension. */


Inaccurate now: it is not just the first search extension, it calls all of them.

Also, there is no such concept as a "search extension". An extension can have any number of query transformers. The relevant concept here is "query transformers".

So:

/** Transforms search queries using registered query transformers from extensions. */

sqs · 2018-10-26T05:13:25Z

src/client/providers/queryTransformer.ts

+ * Returns an observable that emits a query transformed by all providers whenever any of the last-emitted set of providers emits
+ * a query.
+ *
+ * Most callers should use QueryTransformerRegistry's transformQuery method, which uses the registered search


The term "search provider" is inaccurate; there is no such concept (or it is an imprecise way to refer to query transformers). It should be "query transformers".

sqs · 2018-10-26T05:16:19Z

src/client/providers/queryTransformer.ts

+            return providers.reduce(
+                (currentQuery, transformQuery) =>
+                    currentQuery.pipe(
+                        flatMap(q => transformQuery(q).pipe(map(transformedQuery => transformedQuery || q)))


Is this the desired behavior?

If a query transformer returns null or the empty string, use the original input string.

Otherwise, use the original input string.

I suspect you don't intend to consider the empty string as meaning "use the original input string". But the empty string is falsey.

So I would do:

transformedQuery => transformedQuery === null ? q : transformedQuery

Or, really, why special-case null at all? If a query transformer wants to not make any modifications to the string, it can just return it unmodified. That is actually what I'd recommend.

sqs · 2018-10-26T05:20:13Z

src/integration-test/search.test.ts

+            'bar'
+        )
+
+        // Unregister the provider and ensure it's removed


for consistency, add a period after a line comment if it's a full sentence

sqs · 2018-10-26T05:21:00Z

src/integration-test/search.test.ts

+        const { clientController, extensionHost, ready } = await integrationTestContext()
+
+        // Register the provider and call it
+        extensionHost.search.registerQueryTransformer({ transformQuery: q => `${q} bar` })


Minor, but bar and baz look almost the same; it's not easy to visually spot that they are distinct, given that everything else on the 2 lines is the same. I would recommend using foo, qux, etc., here on one of the lines so it's easier to spot that they are different.

sqs · 2018-10-26T05:22:18Z

src/sourcegraph.d.ts

@@ -717,6 +717,14 @@ declare module 'sourcegraph' {
        ): ProviderResult<Location[]>
    }

+    export interface QueryTransformerProvider {


Add tsdoc docstrings to:

interface QueryTransformerProvider

QueryTransformerProvider#transformQuery

namespace search

function registerQueryTransformer

sqs · 2018-10-26T05:29:12Z

src/sourcegraph.d.ts

@@ -717,6 +717,14 @@ declare module 'sourcegraph' {
        ): ProviderResult<Location[]>
    }



I think you should just call it QueryTransformer not QueryTransformerProvider. I struggled with this in other cases but here's my thinking:

The goal of naming is to communicate what something is.

Using the "Provider" suffix is generally good for things like this because it communicates that this fits the "provider" pattern. Not in the strict Erich Gamma Design Patterns sense, but just in the "this is like hover providers, definition providers, etc., in that it can be registered/unregistered and provides data given some parameters".

But when there is a word that is similar to "provider", is more accurate, is short, and is the term you'd use anywhere else in code describing this, then you should use that word instead.

In this case, that word is "Transformer". QueryTransformer and QueryTransformerProvider basically mean the same thing; in the latter, "transformer" and "provider" basically mean the same thing, so it's weird to have them both.

Looking at vscode.d.ts, they have many things that follow the guidelines I laid out instead of always using the "Provider" suffix: registerUriHandler, registerWebviewPanelSerializer, etc. Those are way better than registerUriHandlerProvider or registerUriProvider, or registerWebviewPanelSerializationProvider or registerWebviewPanelSerializerProvider.

sqs · 2018-10-26T05:35:03Z

src/sourcegraph.d.ts

@@ -717,6 +717,14 @@ declare module 'sourcegraph' {
        ): ProviderResult<Location[]>
    }

+    export interface QueryTransformerProvider {
+        transformQuery(query: string): ProviderResult<string>


I wrote 2 comments above about:

not special-casing null return values

using Promise<string> instead of Promise<string | null | undefined>

I see why you included null and undefined now, because ProviderResult adds them. But I think ProviderResult is only useful for things like hover providers, not for transform funcs. So you should just make this return string | Promise<string>.

The reason why ProviderResult is useful for hover/definition/reference/etc. providers is that they need to be able to return null to say "I have no answer for this query". But in transformQuery, you can say "I have no transformations to apply to this query" by just returning the original query input argument.

Another related reason you should use string | Promise<string> instead of ProviderResult<string> here is that it adds confusion if there are 3 things that all mean the same thing ("do not transform this query"):

query => query

query => null

query => undefined

There is no need for the 2nd and 3rd ways of expressing this.

This reverts commit 61756bc.

sqs · 2018-10-26T21:52:28Z

src/sourcegraph.d.ts

+     */
+    export interface QueryTransformer {
+        /**
+         * Transforms a search query into another, valid query. If there are no transformations to be made


I'd replace "the original query is returned" with ", it should return the original query"

sqs · 2018-10-26T21:52:45Z

src/sourcegraph.d.ts

+         * Transforms a search query into another, valid query. If there are no transformations to be made
+         * the original query is returned.
+         *
+         * @param query A search query provided by a client.


just "A search query" is fine, no need to say "provided by a client" (not sure what that adds)

sqs · 2018-10-26T21:54:03Z

src/sourcegraph.d.ts

+        /**
+         * Registers a query transformer.
+         *
+         * Multiple transformers can be registered. In that case, all transformers will be executed


I think it is more idiomatic to say a transformation is applied, not executed. So replace "executed" with "applied".

Also, the order in which multiple transformers are applied is undefined (i.e., the extension API makes no guarantee). So I'd state that.

sourcegraph-bot · 2018-10-26T22:13:41Z

🎉 This PR is included in version 18.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

attfarhan added 5 commits October 22, 2018 13:21

feat: search provider foo

efa285d

feat: can registerSearchProvider

5e4fe5b

fix: clean up

de15667

feat: return original query if no providers

f5fd2be

docs: update docs

e10a9fc

chrismwendt approved these changes Oct 24, 2018

View reviewed changes

attfarhan mentioned this pull request Oct 24, 2018

Modify search to use search extensions sourcegraph/sourcegraph-public-snapshot#499

Merged

attfarhan requested a review from sqs October 24, 2018 05:36

chore: rename SearchProvider to QueryTransformProvider

25ca64b

sqs suggested changes Oct 24, 2018

View reviewed changes

attfarhan added 5 commits October 24, 2018 18:24

fix: improve naming

c76d18c

chore: add tests for queryTransformer

8a1c7aa

fix: remove commented chaining attempts

39590b9

fix: more renames and cleanup

4847f2b

chore: add integration test

13e53ba

sqs mentioned this pull request Oct 25, 2018

Why is viewing a list of repositories that refer to another repository disabled? sourcegraph/sourcegraph-public-snapshot#145

Closed

sqs approved these changes Oct 26, 2018

View reviewed changes

attfarhan added 5 commits October 26, 2018 11:38

fix: address comments

4bbef15

fix: remove unneeded internal tsdoc directives

c112fa7

fix: add string as transform query return type

61756bc

Revert "fix: add string as transform query return type"

f1326e3

This reverts commit 61756bc.

fix: update docs

480071b

sqs reviewed Oct 26, 2018

View reviewed changes

sqs approved these changes Oct 26, 2018

View reviewed changes

fix: docs

4c57cae

attfarhan merged commit 3c629da into master Oct 26, 2018

attfarhan deleted the fa/search-provider branch October 26, 2018 22:10

sqs mentioned this pull request Nov 5, 2018

Extension request: search a repository and its dependencies sourcegraph/sourcegraph-public-snapshot#706

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add search features API #95

Add search features API #95

attfarhan commented Oct 24, 2018

codecov bot commented Oct 24, 2018 •

edited

Loading

chrismwendt left a comment

chrismwendt Oct 24, 2018

attfarhan commented Oct 24, 2018

chrismwendt commented Oct 24, 2018

sqs left a comment

sqs Oct 24, 2018

sqs Oct 24, 2018

attfarhan commented Oct 25, 2018

sqs left a comment

sqs Oct 26, 2018

sqs Oct 26, 2018

sqs Oct 26, 2018

sqs Oct 26, 2018

sqs Oct 26, 2018

sqs Oct 26, 2018

sqs Oct 26, 2018

sqs Oct 26, 2018

sqs Oct 26, 2018

sqs Oct 26, 2018

sqs Oct 26, 2018

sqs Oct 26, 2018

sqs Oct 26, 2018

sourcegraph-bot commented Oct 26, 2018


		export type TransformQuerySignature = (query: string) => Observable<string \| null \| undefined>

		/** Provides transformed queries from the first search extension. */

		@@ -717,6 +717,14 @@ declare module 'sourcegraph' {
		): ProviderResult<Location[]>
		}

Add search features API #95

Add search features API #95

Conversation

attfarhan commented Oct 24, 2018

codecov bot commented Oct 24, 2018 • edited Loading

Codecov Report

chrismwendt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

attfarhan commented Oct 24, 2018

chrismwendt commented Oct 24, 2018

sqs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

attfarhan commented Oct 25, 2018

sqs left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sourcegraph-bot commented Oct 26, 2018

codecov bot commented Oct 24, 2018 •

edited

Loading