-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Replace Jedi with Jedi LSP #17273
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
Replace Jedi with Jedi LSP #17273
Changes from all commits
b4424c1
7993e17
560ea71
68bb632
185d72e
0c12ed3
34ea029
ca5dbff
c48302d
0fd8455
4f5612c
61dad1c
ec0db02
777680e
9044986
6bf6a24
4bafd38
7f5bb1b
bd91ebe
ac36ebd
df2841f
f432385
efdab05
3733b58
d85d0a9
fc85f2a
5418db5
010d3d4
f38558f
b1f3819
54ca6b5
5a3f849
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 |
---|---|---|
@@ -0,0 +1 @@ | ||
Phase out Jedi 0.17, and use Jedi behind a language server protocol as the Jedi option. |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -3,7 +3,7 @@ | |||||
import '../common/extensions'; | ||||||
|
||||||
import { inject, injectable } from 'inversify'; | ||||||
import { ConfigurationChangeEvent, Disposable, OutputChannel, Uri } from 'vscode'; | ||||||
import { ConfigurationChangeEvent, ConfigurationTarget, Disposable, OutputChannel, Uri } from 'vscode'; | ||||||
|
||||||
import { LSNotSupportedDiagnosticServiceId } from '../application/diagnostics/checks/lsNotSupported'; | ||||||
import { IDiagnosticsService } from '../application/diagnostics/types'; | ||||||
|
@@ -56,6 +56,8 @@ export class LanguageServerExtensionActivationService | |||||
|
||||||
private readonly workspaceService: IWorkspaceService; | ||||||
|
||||||
private readonly configurationService: IConfigurationService; | ||||||
|
||||||
private readonly output: OutputChannel; | ||||||
|
||||||
private readonly interpreterService: IInterpreterService; | ||||||
|
@@ -69,6 +71,7 @@ export class LanguageServerExtensionActivationService | |||||
@inject(IPersistentStateFactory) private stateFactory: IPersistentStateFactory, | ||||||
) { | ||||||
this.workspaceService = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService); | ||||||
this.configurationService = this.serviceContainer.get<IConfigurationService>(IConfigurationService); | ||||||
this.interpreterService = this.serviceContainer.get<IInterpreterService>(IInterpreterService); | ||||||
this.output = this.serviceContainer.get<OutputChannel>(IOutputChannel, STANDARD_OUTPUT_CHANNEL); | ||||||
|
||||||
|
@@ -87,8 +90,8 @@ export class LanguageServerExtensionActivationService | |||||
this.serviceContainer.get<IExtensions>(IExtensions), | ||||||
this.serviceContainer.get<IApplicationShell>(IApplicationShell), | ||||||
this.serviceContainer.get<ICommandManager>(ICommandManager), | ||||||
this.serviceContainer.get<IWorkspaceService>(IWorkspaceService), | ||||||
this.serviceContainer.get<IConfigurationService>(IConfigurationService), | ||||||
this.workspaceService, | ||||||
this.configurationService, | ||||||
); | ||||||
disposables.push(this.languageServerChangeHandler); | ||||||
} | ||||||
|
@@ -228,6 +231,9 @@ export class LanguageServerExtensionActivationService | |||||
key: string, | ||||||
): Promise<RefCountedLanguageServer> { | ||||||
let serverType = this.getCurrentLanguageServerType(); | ||||||
|
||||||
this.updateLanguageServerSetting(resource); | ||||||
|
||||||
if (serverType === LanguageServerType.Microsoft) { | ||||||
const lsNotSupportedDiagnosticService = this.serviceContainer.get<IDiagnosticsService>( | ||||||
IDiagnosticsService, | ||||||
|
@@ -243,22 +249,13 @@ export class LanguageServerExtensionActivationService | |||||
} | ||||||
} | ||||||
|
||||||
if (serverType === LanguageServerType.JediLSP && interpreter && interpreter.version) { | ||||||
if (interpreter.version.major < 3 || (interpreter.version.major === 3 && interpreter.version.minor < 6)) { | ||||||
sendTelemetryEvent(EventName.JEDI_FALLBACK); | ||||||
serverType = LanguageServerType.Jedi; | ||||||
} | ||||||
} | ||||||
|
||||||
// If Pylance was chosen via the default and the interpreter is Python 2, fall back to | ||||||
// Jedi. If Pylance was explicitly chosen, continue anyway, even if Pylance won't work | ||||||
// as expected, matching pre-default behavior. | ||||||
if (this.getCurrentLanguageServerTypeIsDefault()) { | ||||||
if (serverType === LanguageServerType.Node && interpreter && interpreter.version) { | ||||||
if (interpreter.version.major < 3) { | ||||||
sendTelemetryEvent(EventName.JEDI_FALLBACK); | ||||||
serverType = LanguageServerType.Jedi; | ||||||
} | ||||||
// If the interpreter is Python 2 and the LS setting is explicitly set to Jedi, turn it off. | ||||||
// If set to Default, use Pylance. | ||||||
if (interpreter && (interpreter.version?.major ?? 0) < 3) { | ||||||
if (serverType === LanguageServerType.Jedi) { | ||||||
serverType = LanguageServerType.None; | ||||||
} else if (this.getCurrentLanguageServerTypeIsDefault()) { | ||||||
serverType = LanguageServerType.Node; | ||||||
} | ||||||
} | ||||||
|
||||||
|
@@ -348,4 +345,23 @@ export class LanguageServerExtensionActivationService | |||||
const values = await Promise.all([...this.cache.values()]); | ||||||
values.forEach((v) => (v.clearAnalysisCache ? v.clearAnalysisCache() : noop())); | ||||||
} | ||||||
|
||||||
private updateLanguageServerSetting(resource: Resource): void { | ||||||
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. Do you think there's a better spot where this could be done? It feels weird updating the settings.json file here. 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. We do similar this in testing too. But we have the code in a separate file. (See Ideally we would have a settings manager and we would register this as a change provider with that. 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 assume this is just for settings migration. We do have a diagnostics convention for stuff like this, ideally
for example. 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.
Yes, it replaces the Looking at the 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'm going to display the "Python 2.7 isn't supported" notification as a diagnostic, so I'm going to move the settings migration there in that PR. 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.
Yes, the point I was making was it could be used for migration. vscode-python/src/client/application/diagnostics/checks/invalidLaunchJsonDebugger.ts Line 136 in 48d66bd
Great! |
||||||
// Update settings.json value to Jedi if it's JediLSP. | ||||||
const settings = this.workspaceService | ||||||
.getConfiguration('python', resource) | ||||||
.inspect<LanguageServerType>('languageServer'); | ||||||
|
||||||
let configTarget: ConfigurationTarget; | ||||||
|
||||||
if (settings?.workspaceValue === LanguageServerType.JediLSP) { | ||||||
configTarget = ConfigurationTarget.Workspace; | ||||||
} else if (settings?.globalValue === LanguageServerType.JediLSP) { | ||||||
configTarget = ConfigurationTarget.Global; | ||||||
} else { | ||||||
return; | ||||||
} | ||||||
|
||||||
this.configurationService.updateSetting('languageServer', LanguageServerType.Jedi, resource, configTarget); | ||||||
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. Is it a good idea to change this? It's no longer a valid enum value but is still accepted by the settings parser. People may have checked in these settings, so I'd be wary of forcibly changing them to something else at startup, but maybe it won't be too many people. 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. We could leave it to show squiggles for a few releases and later remove it? 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. Removing it from package.json will effectively do this, so that's okay. I don't have a strong preference; I know that Pylance has migration code for old MPLS settings, so I guess we're already accepting setting migrations. Have the same thing here already (though in that other test settings migrator thingy). 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. We do migrations like this all the time for |
||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -293,8 +293,12 @@ export class PythonSettings implements IPythonSettings { | |
userLS === 'Default' || | ||
!Object.values(LanguageServerType).includes(userLS as LanguageServerType) | ||
) { | ||
this.languageServer = this.defaultLS?.defaultLSType ?? LanguageServerType.Jedi; | ||
this.languageServer = this.defaultLS?.defaultLSType ?? LanguageServerType.None; | ||
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. Trying to remember, but I believe that there was some test that relied on this staying |
||
this.languageServerIsDefault = true; | ||
} else if (userLS === 'JediLSP') { | ||
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. @jakebailey did you mean this settings parser? 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. Yes, exactly; this is the "untrusted" part where we validate the user's settings before actually casting them to the enum. |
||
// Switch JediLSP option to Jedi. | ||
this.languageServer = LanguageServerType.Jedi; | ||
this.languageServerIsDefault = false; | ||
} else { | ||
this.languageServer = userLS as LanguageServerType; | ||
this.languageServerIsDefault = false; | ||
|
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.
If I have the inner condition the other way around (if default -> node, else if jedi -> none) then the 2.7 debugger tests break 🤷
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.
Reverting the condition can change the language server as the default LS can be Jedi as well, although not sure how intellisense affects debugger 😂
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.
True!
The debugger tests spawn a VS Code instance, it's something about the extension activating and not finding Pylance (not sure why it causes an activation failure instead of displaying the "Pylance is not installed, do you want to install it" message though)