Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hook up JupyterUri handle change #10017

Merged
merged 5 commits into from
May 16, 2022
Merged

Hook up JupyterUri handle change #10017

merged 5 commits into from
May 16, 2022

Conversation

DonJayamanne
Copy link
Contributor

Part of #9683

  • When the onDidChangeHandles event is fired, then look for controllers that belong to jupyter handles that are no longer valid. List of valid handles returned by the provider using the getHandles method
  • When loading cached controllers, ensure they are validated against the getHandles method of the corresponding provider.
    • E.g. when re-loading vscode, the cahched controllers will not be displayed if the handle is outdated.
    • (kind of a moot point caching at this point, as its possible fetching the latest kernels would be fast enough).

@DonJayamanne DonJayamanne requested a review from a team as a code owner May 13, 2022 00:34
@codecov-commenter
Copy link

codecov-commenter commented May 13, 2022

Codecov Report

Merging #10017 (9090f51) into main (1a06230) will increase coverage by 0%.
The diff coverage is n/a.

@@          Coverage Diff           @@
##            main   #10017   +/-   ##
======================================
  Coverage     63%      64%           
======================================
  Files        215      215           
  Lines      10122    10122           
  Branches    1612     1612           
======================================
+ Hits        6473     6481    +8     
+ Misses      3131     3121   -10     
- Partials     518      520    +2     
Impacted Files Coverage Δ
...rc/platform/common/application/encryptedStorage.ts 37% <0%> (-8%) ⬇️
src/platform/errors/errorHandler.ts 62% <0%> (-1%) ⬇️
src/platform/common/process/baseDaemon.node.ts 54% <0%> (+3%) ⬆️
...rc/platform/common/dataScienceSurveyBanner.node.ts 70% <0%> (+5%) ⬆️

@IanMatthewHuff
Copy link
Member

Looks like something in this PR made tests unhappy.

if (this.lastSavedList) {
return this.lastSavedList;
}
const promise = async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(tip for reviewers)
This PR reads much better with whitespaces hidden: https://github.com/microsoft/vscode-jupyter/pull/10017/files?w=1

}
public async clearUriList(): Promise<void> {
this.lastSavedList = Promise.resolve([]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see the cache gets cleared here! Nice

Copy link
Contributor

@sadasant sadasant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty neat!

@DonJayamanne DonJayamanne merged commit d403002 into main May 16, 2022
@DonJayamanne DonJayamanne deleted the hookupJupyterUriHandles branch May 16, 2022 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants