- Notifications
You must be signed in to change notification settings - Fork 1.3k
[RFC] watch pyrefly settings#25041
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
base:main
Are you sure you want to change the base?
[RFC] watch pyrefly settings #25041
Conversation
kinto0 commented May 7, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
kinto0 commented May 7, 2025
@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! |
karthiknadig commented May 8, 2025
@rchiodo can you help with this. I suspect we may need to re-load here. |
rchiodo commented May 8, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
The output looks like it's not even trying to start pylance. Meaning that would be entirely in the Python extension. |
rchiodo commented May 8, 2025
I'll try building this branch and see what I get. |
rchiodo commented May 8, 2025
Yeah this works for me. I just followed the steps in the contributing wiki.
I had pylance just installed like normal. |
rchiodo commented May 8, 2025
When I set this it seems it's not starting Pylance though: Or it's killing it as I get this in our log: |
rchiodo commented May 8, 2025
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 privateasynconDidChangeConfiguration(event: ConfigurationChangeEvent): Promise<void>{constworkspacesUris=this.workspaceService.workspaceFolders?.map((workspace)=>workspace.uri)??[];workspacesUris.forEach(async(resource)=>{if(event.affectsConfiguration(`python.languageServer`,resource)){awaitthis.refreshLanguageServer(resource);<---Thishere}elseif(event.affectsConfiguration(`python.analysis.pylanceLspClientEnabled`,resource)){awaitthis.refreshLanguageServer(resource,/* forced */true);}elseif(event.affectsConfiguration(`python.pyrefly.disableLanguageServices`,resource)){awaitthis.refreshLanguageServer(resource);}});}Then we get another update for the pyrefly setting changes which then causes this to fire: privateasynconDidChangeConfiguration(event: ConfigurationChangeEvent): Promise<void>{constworkspacesUris=this.workspaceService.workspaceFolders?.map((workspace)=>workspace.uri)??[];workspacesUris.forEach(async(resource)=>{if(event.affectsConfiguration(`python.languageServer`,resource)){awaitthis.refreshLanguageServer(resource);}elseif(event.affectsConfiguration(`python.analysis.pylanceLspClientEnabled`,resource)){awaitthis.refreshLanguageServer(resource,/* forced */true);}elseif(event.affectsConfiguration(`python.pyrefly.disableLanguageServices`,resource)){awaitthis.refreshLanguageServer(resource);<---Thishere}});}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. |
kinto0 commented May 8, 2025
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 |
rchiodo commented May 16, 2025
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. |
kinto0 commented May 16, 2025
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). |
rchiodo commented May 16, 2025
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. |
rchiodo commented May 16, 2025
@karthiknadig do you know how the Python extension indicates 'restart' required on install? Maybe pyrefly can just set that same setting |
karthiknadig commented May 16, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@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. |
rchiodo commented May 16, 2025
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 |
karthiknadig commented May 23, 2025
@kinto0 Can you add this event or do you need help with this? |
kinto0 commented May 23, 2025
sorry about my delay. we have such a big backlog of pyrefly bugs ahead of us from the release. Unfortunately, this one is low on the list because it actually does work now with my workaround. It's more of a better engineering task. If you think it's important for pylance (example: someone else wants to add a LS to pylance with a similar setup) and want to help out , please do! |



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: