Move search function to use Promise interface #32609

kgrz · 2017-08-16T05:30:19Z

Draft to fix #20650

Initial draft of changes for moving the search function inside FileSearch and
TextSearch modules to ause a promise interface instead of callbacks.

I have a couple of problems with the changes I made:

The complete action's object shape is very different from that of
progress. So much so that the usage doesn't feel natural to me.

As @roblourens pointed out, may be it's time to have different interfaces
for text and file search.
This doesn't clean up code related to ripgrepsearch yet.

mention-bot · 2017-08-16T05:30:22Z

@kgrz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @roblourens and @bpasero to be potential reviewers.

roblourens

Thanks for tackling this! It looks like a great start to me.

The complete action's object shape is very different from that of progress. So much so that the usage doesn't feel natural to me.

Is that a problem? I would expect them to be different and carry different information

roblourens · 2017-08-17T04:58:07Z

src/vs/workbench/services/search/node/fileSearch.ts

-			});
+	search(): PPromise<ISerializedSearchComplete, ISearchProgress<IRawFileMatch>> {
+		return new PPromise<ISerializedSearchComplete, ISearchProgress<IRawFileMatch>>((complete, error, progress) => {
+			this.walker.walk(this.folderQueries, this.extraFiles,


I think it would make sense for walk to return a PPromise as well.

Oh! yeah, I forgot to mention this in the PR description. This usage was very odd. Will try migrating this too.

roblourens · 2017-08-17T05:18:27Z

Looks like there is a search-related test failing.

kgrz · 2017-08-17T05:49:14Z

Weird. I didn't see this test run output when I ran on my local machine (OSX). Will recheck.

rocketberg · 2017-09-27T17:02:33Z

src/vs/workbench/services/search/node/fileSearch.ts

-				limitHit: isLimitHit,
-				stats: this.walker.getStats()
-			});
+	search(): PPromise<ISerializedSearchComplete, ISearchProgress<IRawFileMatch>> {


You forgot access modificator

Oh! thanks for pointing this out. Corrected.

rocketberg · 2017-09-27T17:04:30Z

src/vs/workbench/services/search/node/fileSearch.ts

+	search(): PPromise<ISerializedSearchComplete, ISearchProgress<IRawFileMatch>> {
+		return new PPromise<ISerializedSearchComplete, ISearchProgress<IRawFileMatch>>((complete, error, progress) => {
+			this.walker.walk(this.folderQueries, this.extraFiles,
+				(result: IRawFileMatch) => {


Why you use function body? You can write is like: (result: IRawFileMatch) => progress({ results: result });
In one line.

There's a subtle difference between calling a function and returning the result, especially in the case of promises. I wanted to keep the old structure.

rocketberg · 2017-09-27T17:07:47Z

src/vs/workbench/services/search/node/rawSearchService.ts

-			engine.search((matches) => {
-				const totalMatches = matches.reduce((acc, m) => acc + m.numMatches, 0);
-				collector.addItems(matches, totalMatches);
+			engine.search().then((success) => {


Can you specify type for success if you don't specify type you aren't needed parentheses

rocketberg · 2017-09-27T17:08:39Z

src/vs/workbench/services/search/node/rawSearchService.ts

-				} else {
-					c(stats);
-				}
+				const results = progress.results;


Which type of const results. Can you specify this type?

Same question as below. I'm leaning towards not specifying these implicit types everywhere.

rocketberg · 2017-09-27T17:10:52Z

src/vs/workbench/services/search/node/rawSearchService.ts

-					c(stats);
-				}
+				const results = progress.results;
+				const totalMatches = results.reduce((acc, m) => acc + m.numMatches, 0);


Which type of const totalMatches. Can you specify this?
I think within parentheses you can specify type.

Do we need to specify types for automatically inferred primitives? Because we aren't implementing reduce, and the return type of that will never change (by our actions).

No, I don't specify the type when it isn't necessary. This is fine.

roblourens · 2017-11-12T02:01:55Z

Thanks, but do you know why the tests are failing?

TODO: There's a bit of duplication inside the `complete` and `error` callbacks.

kgrz · 2017-11-12T04:20:14Z

@roblourens I'm sort of reworking this today. Will update once done.

kgrz · 2017-11-12T04:27:01Z

Tests are failing because I made a mess 😐 I didn't change some of the search functions in tests to the promise format.

This will combine the result and the progress metrics

* Update integration tests accordingly.

kgrz · 2017-11-13T15:29:12Z

@roblourens I've reworked this changeset, this time preserving the old search and walk APIs for comparison. There are three unit tests failing, all related to the extraFiles config param in fileSearch tests. I'm unable figure this failure out. I tried increasing the timeouts, I did as much printf-debugging as possible and can confirm that the progress callback is being called. This leaves one possibility that this might be some race condition—either in the code itself, or the way the test is written. I might need a bit of help or pointers here.

To be more precise, the onResult inside the this.matchFile gets called, but this line in searchP never gets called. May be my mental modelling for this API is wrong.

roblourens · 2017-11-20T23:53:16Z

Thanks, I promise I'll look at this when I have a minute.

roblourens · 2018-01-03T21:37:05Z

Sorry for the delay. The tests are failing because of a tricky point with PPromise - if the progress callback is called synchronously inside the Promise constructor, then the progress call will be lost, because the caller's progress callback will not be attached. To work around this, you can wait a tick to give the caller a chance to attach the progress callback. We do this in several places. Here's one example: https://github.com/Microsoft/vscode/blob/4173c6478d4ac630fa4064485ecec610a02b8b5c/src/vs/workbench/services/search/node/searchService.ts#L88

That should be it. Besides that, there are just a couple things to do before I merge this -

Fix merge conflict - It doesn't look serious
Remove the previous implementations of these methods, we don't need both
Rename the new implementations to have the same name as the previous - e.g. searchP -> search

roblourens · 2018-05-15T18:57:38Z

I'm largely rewriting this code as part of #47058

msftclas added the cla-required label Aug 16, 2017

kgrz mentioned this pull request Aug 16, 2017

Use promises in SearchEngine #20650

Closed

aeschli assigned roblourens Aug 16, 2017

msftclas added cla-signed and removed cla-required labels Aug 16, 2017

roblourens reviewed Aug 17, 2017

View reviewed changes

msftgits removed the cla-signed label Sep 26, 2017

microsoft deleted a comment from msftclas Sep 26, 2017

rocketberg suggested changes Sep 27, 2017

View reviewed changes

kgrz force-pushed the move-search-to-promises branch from a11d4cb to 207f678 Compare November 11, 2017 07:14

kgrz added 2 commits November 12, 2017 09:28

Add a new promisified walk function: walkP

45ca26a

Use promisified walk in textSearch.search

26b2689

TODO: There's a bit of duplication inside the `complete` and `error` callbacks.

kgrz added 6 commits November 12, 2017 13:22

Use new walkP in fileSearch.search

1a6b1ec

Add promisified search function

502f287

Change tests to use new promisified search function

7d49af4

Add new generic type for progress metrics

0c4b993

This will combine the result and the progress metrics

Move textSearch#search to a promise

49f0c4c

* Update integration tests accordingly.

Move file walk start timestamp out of promise

ed5a579

kgrz force-pushed the move-search-to-promises branch from 9a885d4 to ed5a579 Compare November 13, 2017 15:24

roblourens closed this May 15, 2018

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move search function to use Promise interface #32609

Move search function to use Promise interface #32609

kgrz commented Aug 16, 2017

mention-bot commented Aug 16, 2017

roblourens left a comment

roblourens Aug 17, 2017

kgrz Aug 17, 2017

roblourens commented Aug 17, 2017

kgrz commented Aug 17, 2017

rocketberg Sep 27, 2017

kgrz Nov 11, 2017

rocketberg Sep 27, 2017

kgrz Nov 11, 2017

rocketberg Sep 27, 2017

rocketberg Sep 27, 2017

kgrz Nov 11, 2017

rocketberg Sep 27, 2017

kgrz Nov 11, 2017

roblourens Nov 12, 2017

roblourens commented Nov 12, 2017

kgrz commented Nov 12, 2017

kgrz commented Nov 12, 2017

kgrz commented Nov 13, 2017 •

edited

Loading

roblourens commented Nov 20, 2017

roblourens commented Jan 3, 2018 •

edited

Loading

roblourens commented May 15, 2018

Move search function to use Promise interface #32609

Move search function to use Promise interface #32609

Conversation

kgrz commented Aug 16, 2017

mention-bot commented Aug 16, 2017

roblourens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roblourens commented Aug 17, 2017

kgrz commented Aug 17, 2017

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

roblourens commented Nov 12, 2017

kgrz commented Nov 12, 2017

kgrz commented Nov 12, 2017

kgrz commented Nov 13, 2017 • edited Loading

roblourens commented Nov 20, 2017

roblourens commented Jan 3, 2018 • edited Loading

roblourens commented May 15, 2018

kgrz commented Nov 13, 2017 •

edited

Loading

roblourens commented Jan 3, 2018 •

edited

Loading