[RFC] watch pyrefly settings #25041
[RFC] watch pyrefly settings #25041
Conversation
|
@karthiknadig I still can't get pylance ever started locally to test. Would you be able to assist? this is all I see in the output pane: Rich mentioned here that pylance needs to be installed in the extensions folder. I don't have pylance source so I can't get that one, but I do have the release pylance there (because it's installed normally). I also tried cloning this repo into the extensions folder but that made no difference. Thanks! |
|
@rchiodo can you help with this. I suspect we may need to re-load here. |
|
The output looks like it's not even trying to start pylance. Meaning that would be entirely in the Python extension. |
|
I'll try building this branch and see what I get. |
|
Yeah this works for me. I just followed the steps in the contributing wiki.
I had pylance just installed like normal. |
|
When I set this it seems it's not starting Pylance though:
Or it's killing it as I get this in our log: |
|
I did some debugging. The changing of this setting seems to set off a cascade of events:
First this fires: this.disposables.push(
this.workspace.onDidChangeConfiguration((event: ConfigurationChangeEvent) => {
if (event.affectsConfiguration('python')) {
this.onDidChanged(event);
}
}),
);Well because the pyrefly setting is prefixed with python. That causes the internal 'update' method to get called. That in turn changes the 'languageServer' setting to a new value, which then causes another update. That then causes this to fire private async onDidChangeConfiguration(event: ConfigurationChangeEvent): Promise<void> {
const workspacesUris = this.workspaceService.workspaceFolders?.map((workspace) => workspace.uri) ?? [];
workspacesUris.forEach(async (resource) => {
if (event.affectsConfiguration(`python.languageServer`, resource)) {
await this.refreshLanguageServer(resource); <--- This here
} else if (event.affectsConfiguration(`python.analysis.pylanceLspClientEnabled`, resource)) {
await this.refreshLanguageServer(resource, /* forced */ true);
} else if (event.affectsConfiguration(`python.pyrefly.disableLanguageServices`, resource)) {
await this.refreshLanguageServer(resource);
}
});
}Then we get another update for the pyrefly setting changes which then causes this to fire: private async onDidChangeConfiguration(event: ConfigurationChangeEvent): Promise<void> {
const workspacesUris = this.workspaceService.workspaceFolders?.map((workspace) => workspace.uri) ?? [];
workspacesUris.forEach(async (resource) => {
if (event.affectsConfiguration(`python.languageServer`, resource)) {
await this.refreshLanguageServer(resource);
} else if (event.affectsConfiguration(`python.analysis.pylanceLspClientEnabled`, resource)) {
await this.refreshLanguageServer(resource, /* forced */ true);
} else if (event.affectsConfiguration(`python.pyrefly.disableLanguageServices`, resource)) {
await this.refreshLanguageServer(resource); <--- This here
}
});
}Which in effect doesn't turn off the language server. Well it actually depends upon what the setting was on startup. It either turns it on and then off again or turns it on twice. I think the python extension needs to be changed to not double fire this event. |
Are you sure this second update "This here" isn't caused by my patch? To test these changed, I think we need to be on an older version of pyrefly. I |
Yeah, that made things a lot easier to get working. I pushed an update which I think should fix the problem. At least for me locally, with using pyrefly 0.12.1, the setting works as designed. |
Awesome! Thanks for the fix. I can test in the morning. Did this fix the issue for both settings updates and on Pyrefly install? No rush in getting this in until the next release. I'll just remove my pyrefly patch once these go into this extension (everything works fine right now with the hack). |
|
Sorry I didn't realize there was a problem on install. That would likely have to be handled with something waiting for install of pyrefly. Yeah install requires a restart of the extension host for the logic to work. |
|
@karthiknadig do you know how the Python extension indicates 'restart' required on install? Maybe pyrefly can just set that same setting |
|
@rchiodo I think this has to be handled in the on change handler. Some time ago we used to request reload of the extension on language server change by popping up a notification, and later triggering the reload window command. I think we removed it after we made it so you did not need to reload. |
|
Yeah I looked at the VS code source. Restart Extensions isn't controlled by the extension at all. VS code decides based on if the extension is already running etc. So we'd have to listen to this event: https://github.com/microsoft/vscode/blob/2d6afddc470bf44f7e60fb5b6e6fdd08e771409b/src/vscode-dts/vscode.d.ts#L17320-L17321 |
I tested my patch in pre-release this morning but it only works after a window-reload. It doesn't work in the following cases:
20250507-1509-58.9438483.mp4
20250507-1508-53.1954191.mp4
In the PR, I tested the config settings but not the watch behavior because I couldn't get Pylance working locally.
The first issue makes sense to me: I'm pretty sure I should've kept this (fixed in this PR).
The second issue does not: we refresh on settings changes assuming this.languageServer != the new one.
I'm still not able to test this locally: my windows machine is failing to compile right now. I will have access to a mac later today, but I will likely have the same issues testing it as last time. In the meantime, I've patched pyrefly to change
python.languageServerthen change it back on activation and changes to the disableLanguageServices setting (patch).I'm looking for advice: