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

Avoid validation of Jupyter Server providers #14081

Merged
merged 6 commits into from
Aug 8, 2023

Conversation

DonJayamanne
Copy link
Contributor

For #13894

currentServers.forEach(this.createRemoteKernelFinder.bind(this));
.then(async (currentServers) => {
await Promise.all(
currentServers.map(async (server) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the only place that required validation of the jupyte servers.
All other calls to getAll didn't require a validation.

return;
}
try {
await this.jupyterPickerRegistration.getJupyterServerUri(item.serverHandle, true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only required in one calling place, removed this and move the validation to the calling code.
Thereby removing a cycling reference and more layers.
This also makes listing of Jupyter Server much faster, as we no longer validate each server, which in turn waits for all related extensions to completely activate.

@DonJayamanne DonJayamanne marked this pull request as ready for review August 8, 2023 12:32
@DonJayamanne DonJayamanne enabled auto-merge (squash) August 8, 2023 12:32
@DonJayamanne DonJayamanne marked this pull request as draft August 8, 2023 12:44
auto-merge was automatically disabled August 8, 2023 12:44

Pull request was converted to draft

}
public async clear(): Promise<void> {
const all = await this.getAllImpl(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

false is the only value supported now.

@@ -219,35 +218,30 @@ suite('Server Uri Storage', async () => {
.sort((a, b) => a.time - b.time)
.map((a) => {
return {
displayName: a.displayName,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

displayName is deprecated

//Validate each of the servers
try {
const info = await this.jupyterPickerRegistration.getJupyterServerUri(serverUri.provider, true);
this.createRemoteKernelFinder(serverUri.provider, info.displayName);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that we moved validation all the way up, we can get the display name after we validate.
this makes displayName obsolete in the storage

@@ -184,6 +184,7 @@ export interface IJupyterServerUriEntry {
time: number;
/**
* An optional display name to show for this server as opposed to just the Uri
* @deprecated Used only for migration of display names into the User Provided Server list. Else other providers will have the Display Names.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

displayName is deprecated, only used for migration of data from one storage into another

@DonJayamanne DonJayamanne marked this pull request as ready for review August 8, 2023 13:11
@DonJayamanne DonJayamanne enabled auto-merge (squash) August 8, 2023 15:14
@DonJayamanne DonJayamanne merged commit cfe6615 into main Aug 8, 2023
30 checks passed
@DonJayamanne DonJayamanne deleted the fasterCreationOfRemoteKernleFinder branch August 8, 2023 15:39
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.

2 participants