Add an API to report progress of environment discovery in two phases #19125
Conversation
This reverts commit 5bbea78.
|
|
||
| /** Keeps track of scheduled refreshes other than the ongoing one for various queries. */ | ||
| private scheduledRefreshes = new Map<PythonLocatorQuery | undefined, Promise<void>>(); | ||
|
|
||
| private readonly refreshStarted = new EventEmitter<void>(); | ||
| private refreshStageDeferreds = new Map<ProgressReportStage, Deferred<void>>(); |
There was a problem hiding this comment.
Why is there one deferred promise per stage? They also seem to not be explicitly tied to the list of refreshes per query in refreshDeferreds, you have to look at createProgressStates/rejectProgressStates/resolveProgressStates to understand that they are linked.
There was a problem hiding this comment.
Why is there one deferred promise per stage?
The promise for ProgressReportStage.allPathsDiscovered resolves faster the promise for stage ProgressReportStage.discoveryFinished, as all paths are discovered much before the entire refresh is finished. So each stage has to have a different deferred promise.
We're supposed to return this information via the #19125 (comment) API, which is why these promises are tracked. Let me know it makes sense.
There was a problem hiding this comment.
you have to look at createProgressStates/rejectProgressStates/resolveProgressStates to understand that they are linked
Why do you think they're tied? refreshStageDeferreds is meant to carry promise for each stage, whereas refreshDeferreds is meant to carry promise for each query.
There was a problem hiding this comment.
I have renamed the variables and added a couple of comments, hopefully it makes it clearer.
There was a problem hiding this comment.
I guess my biggest question is that, if you can have multiple queries, why aren't we keeping track of one set of stage per query?
There was a problem hiding this comment.
For simplicity, because it isn't needed at the moment. Let's take ProgressReportStage.discoveryFinished for example.
The consumer of envsCollectionService.ts only cares when the collection stops refreshing (when the discovery for all the queries finishes), it's not interested in when discovery finishes for each type of query. Hence stages are only tracked for the "collective" query.
I'm interested if you've a different perspective on this.
There was a problem hiding this comment.
If we feel consumers have a need for tracking progress for a particular query, we can always add it later, but it'll probably make the implementation more complex.
There was a problem hiding this comment.
Sounds good, makes sense. Where is it documented?
There was a problem hiding this comment.
It's documented in the IDiscoveryAPI.onProgress interface, I have amended it with a note.
| public getRefreshPromise(options?: GetRefreshEnvironmentsOptions): Promise<void> | undefined { | ||
| const stage = options?.stage ?? ProgressReportStage.discoveryFinished; | ||
| return this.refreshStageDeferreds.get(stage)?.promise; | ||
| } |
There was a problem hiding this comment.
Add supports to return promises for each stage, indicating when discovery resolves for each stage.
…icrosoft/vscode-python#19125) * Add progress API * Remove old API in favor of this * Revert "Remove old API in favor of this" This reverts commit 9543501576cd063e6aad1ef3ddd7a39cb3354b22. * Revert "Revert "Remove old API in favor of this"" This reverts commit 5bbea78879f2447c6783d7ad98610d4af36abf29. * Fix * Remove old API impl * Add to proposed API * Add get refresh promise options * Change getter to function * Translate progress events to progress promises * Fix combining iterators * Fix tests * Add test for resolver * Add test for reducer * Fixes * Add tests for environment collection service * News entry * Add another test if a query is provided * Add comments and clarify * Fix bug * Add clarifying comment
Closes #19103