-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add "Default" LS setting, which picks between Jedi/Pylance #16139
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1225,13 +1225,14 @@ | |
| "python.languageServer": { | ||
| "type": "string", | ||
| "enum": [ | ||
| "Default", | ||
| "Jedi", | ||
| "JediLSP", | ||
| "Pylance", | ||
| "Microsoft", | ||
| "None" | ||
| ], | ||
| "default": "Jedi", | ||
| "default": "Default", | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should I remove this change now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems fine for now, why do you think this needs to be removed?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one can probably stay regardless, but the other code makes Pylance the default (and IDK when we want to actually flip that switch; this PR or another. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| "description": "Defines type of the language server.", | ||
| "scope": "window" | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import { injectable } from 'inversify'; | ||
| import { PYLANCE_EXTENSION_ID } from '../../common/constants'; | ||
| import { JediLSP } from '../../common/experiments/groups'; | ||
| import { IDefaultLanguageServer, IExperimentService, IExtensions } from '../../common/types'; | ||
| import { IServiceManager } from '../../ioc/types'; | ||
| import { ILSExtensionApi } from '../node/languageServerFolderService'; | ||
| import { LanguageServerType } from '../types'; | ||
|
|
||
| export type PotentialDefault = LanguageServerType.Jedi | LanguageServerType.JediLSP | LanguageServerType.Node; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need not be exported.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch; this should be with |
||
|
|
||
| @injectable() | ||
| class DefaultLanguageServer implements IDefaultLanguageServer { | ||
| public readonly defaultLSType: PotentialDefault; | ||
|
|
||
| constructor(defaultServer: PotentialDefault) { | ||
| this.defaultLSType = defaultServer; | ||
| } | ||
| } | ||
|
|
||
| export async function setDefaultLanguageServer( | ||
| experimentService: IExperimentService, | ||
| extensions: IExtensions, | ||
| serviceManager: IServiceManager, | ||
| ): Promise<void> { | ||
| const lsType = await getDefaultLanguageServer(experimentService, extensions); | ||
| serviceManager.addSingletonInstance<IDefaultLanguageServer>( | ||
| IDefaultLanguageServer, | ||
| new DefaultLanguageServer(lsType), | ||
| ); | ||
| } | ||
|
|
||
| async function getDefaultLanguageServer( | ||
| experimentService: IExperimentService, | ||
| extensions: IExtensions, | ||
| ): Promise<PotentialDefault> { | ||
| if (extensions.getExtension<ILSExtensionApi>(PYLANCE_EXTENSION_ID)) { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would have liked to call We'll have to rely on later checks to fallback to Jedi. |
||
| return LanguageServerType.Node; | ||
| } | ||
|
|
||
| return (await experimentService.inExperiment(JediLSP.experiment)) | ||
|
kimadeline marked this conversation as resolved.
|
||
| ? LanguageServerType.JediLSP | ||
| : LanguageServerType.Jedi; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -167,7 +167,7 @@ export class PythonSettings implements IPythonSettings { | |
| private readonly experimentsManager?: IExperimentsManager, | ||
| private readonly interpreterPathService?: IInterpreterPathService, | ||
| private readonly interpreterSecurityService?: IInterpreterSecurityService, | ||
| private readonly defaultJedi?: IDefaultLanguageServer, | ||
| private readonly defaultLS?: IDefaultLanguageServer, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The meaning of |
||
| ) { | ||
| this.workspace = workspace || new WorkspaceService(); | ||
| this.workspaceRoot = workspaceFolder; | ||
|
|
@@ -181,7 +181,7 @@ export class PythonSettings implements IPythonSettings { | |
| experimentsManager?: IExperimentsManager, | ||
| interpreterPathService?: IInterpreterPathService, | ||
| interpreterSecurityService?: IInterpreterSecurityService, | ||
| defaultJedi?: IDefaultLanguageServer, | ||
| defaultLS?: IDefaultLanguageServer, | ||
| ): PythonSettings { | ||
| workspace = workspace || new WorkspaceService(); | ||
| const workspaceFolderUri = PythonSettings.getSettingsUriAndTarget(resource, workspace).uri; | ||
|
|
@@ -195,7 +195,7 @@ export class PythonSettings implements IPythonSettings { | |
| experimentsManager, | ||
| interpreterPathService, | ||
| interpreterSecurityService, | ||
| defaultJedi, | ||
| defaultLS, | ||
| ); | ||
| PythonSettings.pythonSettings.set(workspaceFolderKey, settings); | ||
| // Pass null to avoid VSC from complaining about not passing in a value. | ||
|
|
@@ -284,14 +284,21 @@ export class PythonSettings implements IPythonSettings { | |
|
|
||
| this.useIsolation = systemVariables.resolveAny(pythonSettings.get<boolean>('useIsolation', true))!; | ||
|
|
||
| const defaultServer = this.defaultJedi | ||
| ? this.defaultJedi.defaultLSType | ||
| : pythonSettings.get<LanguageServerType>('languageServer'); | ||
| let ls = defaultServer ?? LanguageServerType.Jedi; | ||
| ls = systemVariables.resolveAny(ls); | ||
| if (!Object.values(LanguageServerType).includes(ls)) { | ||
| ls = LanguageServerType.Jedi; | ||
| // Get as a string and verify; don't just accept. | ||
| let userLS = pythonSettings.get<string>('languageServer'); | ||
| userLS = systemVariables.resolveAny(userLS); | ||
|
|
||
| let ls: LanguageServerType; | ||
| if ( | ||
| !userLS || | ||
| userLS === 'Default' || | ||
| !Object.values(LanguageServerType).includes(userLS as LanguageServerType) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what this condition signifies. Does this mean if user has selected an invalid LS value, we select the default LS in those cases?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct. We can't trust user values from enums, so this checks for invalid ones and then selects the default if they are present. That's sort of the problem with the VS code settings API; it's too easy to make mistakes. |
||
| ) { | ||
| ls = this.defaultLS?.defaultLSType ?? LanguageServerType.Jedi; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you removed the condition in
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried making it required, and hit a bunch of places that simply don't have access to the data needed, so IDK what's going on there. |
||
| } else { | ||
| ls = userLS as LanguageServerType; | ||
| } | ||
|
|
||
| this.languageServer = ls; | ||
|
|
||
| this.jediPath = systemVariables.resolveAny(pythonSettings.get<string>('jediPath'))!; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| 'use strict'; | ||
|
|
||
| import { expect } from 'chai'; | ||
| import { anything, instance, mock, when, verify } from 'ts-mockito'; | ||
| import { Extension } from 'vscode'; | ||
| import { setDefaultLanguageServer } from '../../client/activation/common/defaultlanguageServer'; | ||
| import { LanguageServerType } from '../../client/activation/types'; | ||
| import { PYLANCE_EXTENSION_ID } from '../../client/common/constants'; | ||
| import { JediLSP } from '../../client/common/experiments/groups'; | ||
| import { ExperimentService } from '../../client/common/experiments/service'; | ||
| import { IDefaultLanguageServer, IExperimentService, IExtensions } from '../../client/common/types'; | ||
| import { ServiceManager } from '../../client/ioc/serviceManager'; | ||
| import { IServiceManager } from '../../client/ioc/types'; | ||
|
|
||
| suite('Activation - setDefaultLanguageServer()', () => { | ||
| let experimentService: IExperimentService; | ||
| let extensions: IExtensions; | ||
| let extension: Extension<unknown>; | ||
| let serviceManager: IServiceManager; | ||
| setup(() => { | ||
| experimentService = mock(ExperimentService); | ||
| extensions = mock(); | ||
| extension = mock(); | ||
| serviceManager = mock(ServiceManager); | ||
| }); | ||
|
|
||
| test('Pylance not installed and NOT in experiment', async () => { | ||
| let defaultServerType; | ||
|
|
||
| when(extensions.getExtension(PYLANCE_EXTENSION_ID)).thenReturn(undefined); | ||
| when(experimentService.inExperiment(JediLSP.experiment)).thenResolve(false); | ||
| when(serviceManager.addSingletonInstance<IDefaultLanguageServer>(IDefaultLanguageServer, anything())).thenCall( | ||
| (_symbol, value: IDefaultLanguageServer) => { | ||
| defaultServerType = value.defaultLSType; | ||
| }, | ||
| ); | ||
|
|
||
| await setDefaultLanguageServer(instance(experimentService), instance(extensions), instance(serviceManager)); | ||
|
|
||
| verify(extensions.getExtension(PYLANCE_EXTENSION_ID)).once(); | ||
| verify(experimentService.inExperiment(JediLSP.experiment)).once(); | ||
| verify(serviceManager.addSingletonInstance<IDefaultLanguageServer>(IDefaultLanguageServer, anything())).once(); | ||
| expect(defaultServerType).to.equal(LanguageServerType.Jedi); | ||
| }); | ||
|
|
||
| test('Pylance not installed and in experiment', async () => { | ||
| let defaultServerType; | ||
| when(extensions.getExtension(PYLANCE_EXTENSION_ID)).thenReturn(undefined); | ||
| when(experimentService.inExperiment(JediLSP.experiment)).thenResolve(true); | ||
| when(serviceManager.addSingletonInstance<IDefaultLanguageServer>(IDefaultLanguageServer, anything())).thenCall( | ||
| (_symbol, value: IDefaultLanguageServer) => { | ||
| defaultServerType = value.defaultLSType; | ||
| }, | ||
| ); | ||
|
|
||
| await setDefaultLanguageServer(instance(experimentService), instance(extensions), instance(serviceManager)); | ||
|
|
||
| verify(extensions.getExtension(PYLANCE_EXTENSION_ID)).once(); | ||
| verify(experimentService.inExperiment(JediLSP.experiment)).once(); | ||
| verify(serviceManager.addSingletonInstance<IDefaultLanguageServer>(IDefaultLanguageServer, anything())).once(); | ||
| expect(defaultServerType).to.equal(LanguageServerType.JediLSP); | ||
| }); | ||
|
|
||
| test('Pylance installed and NOT in experiment', async () => { | ||
| let defaultServerType; | ||
|
|
||
| when(extensions.getExtension(PYLANCE_EXTENSION_ID)).thenReturn(instance(extension)); | ||
| when(experimentService.inExperiment(JediLSP.experiment)).thenResolve(false); | ||
| when(serviceManager.addSingletonInstance<IDefaultLanguageServer>(IDefaultLanguageServer, anything())).thenCall( | ||
| (_symbol, value: IDefaultLanguageServer) => { | ||
| defaultServerType = value.defaultLSType; | ||
| }, | ||
| ); | ||
|
|
||
| await setDefaultLanguageServer(instance(experimentService), instance(extensions), instance(serviceManager)); | ||
|
|
||
| verify(extensions.getExtension(PYLANCE_EXTENSION_ID)).once(); | ||
| verify(experimentService.inExperiment(JediLSP.experiment)).never(); | ||
| verify(serviceManager.addSingletonInstance<IDefaultLanguageServer>(IDefaultLanguageServer, anything())).once(); | ||
| expect(defaultServerType).to.equal(LanguageServerType.Node); | ||
| }); | ||
|
|
||
| test('Pylance installed and in experiment', async () => { | ||
| let defaultServerType; | ||
| when(extensions.getExtension(PYLANCE_EXTENSION_ID)).thenReturn(instance(extension)); | ||
| when(experimentService.inExperiment(JediLSP.experiment)).thenResolve(true); | ||
| when(serviceManager.addSingletonInstance<IDefaultLanguageServer>(IDefaultLanguageServer, anything())).thenCall( | ||
| (_symbol, value: IDefaultLanguageServer) => { | ||
| defaultServerType = value.defaultLSType; | ||
| }, | ||
| ); | ||
|
|
||
| await setDefaultLanguageServer(instance(experimentService), instance(extensions), instance(serviceManager)); | ||
|
|
||
| verify(extensions.getExtension(PYLANCE_EXTENSION_ID)).once(); | ||
| verify(experimentService.inExperiment(JediLSP.experiment)).never(); | ||
| verify(serviceManager.addSingletonInstance<IDefaultLanguageServer>(IDefaultLanguageServer, anything())).once(); | ||
| expect(defaultServerType).to.equal(LanguageServerType.Node); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this block could use an
enumDescriptionsto define the semantics of each of these.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you go: #16141