Detect new virtual environments #7969
Conversation
| .catch((ex) => console.error('Failed to fetch controllers without cache', ex)) | ||
| .finally(() => { | ||
| let timer: NodeJS.Timeout | number | undefined; | ||
| this.interpreters.onDidChangeInterpreters( |
There was a problem hiding this comment.
This is the main change
There was a problem hiding this comment.
How come you're not just listening to the event? This is weird. Why does it need a timer?
There was a problem hiding this comment.
- We only care about the event after we've listed the first set of kernels
- Else during discovery they can fire this 100s of times
- Based on my communication with Kartik this even will get triggered when ever there are changes to an interpreter while discovering them
- Timer is a dirty approach to throttling
- If this event gets fired again, we cancel the previous timer & defer discovery (as thats slow)
(as thats slow)Note: this is hacky, better fix is to refactor all of this and do it in a more efficient manner
There was a problem hiding this comment.
So really you're just debouncing? Maybe put a comment saying how the event fires a whole bunch at the start.
| this.listKernelsImplementation(resource, cancelToken) | ||
| return this.listKernelsWithCache( | ||
| workspaceFolderId, | ||
| true, |
There was a problem hiding this comment.
Discover kernelspecs in the new virtual environments as well (not sure this is required, but added that for completeness)
| @@ -4,6 +4,7 @@ import { PythonEnvironment } from '../pythonEnvironments/info'; | |||
| export const IInterpreterService = Symbol('IInterpreterService'); | |||
| export interface IInterpreterService { | |||
| onDidChangeInterpreter: Event<void>; | |||
| onDidChangeInterpreters: Event<void>; | |||
There was a problem hiding this comment.
This is the new API from python extension
| ): Promise<(KernelSpecConnectionMetadata | PythonKernelConnectionMetadata)[]> { | ||
| // If we have already searched for this resource, then use that. | ||
| const result = this.kernelSpecCache.get(cacheKey); | ||
| if (result) { | ||
| if (result && !ignoreCache) { |
There was a problem hiding this comment.
Maybe I'm reading this wrong. It seems backwards. The comment says if python extension is now installed, then ignore cache.
There was a problem hiding this comment.
Yes, thats correct.
What we're doing here is:
- If python extension was not installed previously and this method was called, then we would have cached it
- Now if we call this method again after python extensoin is installed then
isPythonExtensionIstalled = true&wasPythonExtensionInstalled = false - In this case we should ignore the cache (i.e. if both values are the same, then no changes, & use the cache).
Happy to update with better commands ( I think you should provide some comments I could add or provide some tips on refactoring this so that its more readable to you when you look at this next time)
There was a problem hiding this comment.
Maybe just rename the 'result' variable here to 'cachedResult'. Rereading it now it makes sense because on line 62 it's getting the result from the cache.
Related to #5319