Skip to content

[ty] Implement workspace/didChangeConfiguration#24712

Open
pharmac1st wants to merge 8 commits intoastral-sh:mainfrom
pharmac1st:pgm/workspace-did-change-configuration
Open

[ty] Implement workspace/didChangeConfiguration#24712
pharmac1st wants to merge 8 commits intoastral-sh:mainfrom
pharmac1st:pgm/workspace-did-change-configuration

Conversation

@pharmac1st
Copy link
Copy Markdown

Summary

Implements astral-sh/ty#953

Adds a notification handler for workspace/didConfigurationChange. Re initializes workspaces when this is received with the newly pulled settings.

Workspaces are just reinitialized with a new settings passed from the client.

Copied code from #20545, so credit to @Guillaume-Fgt for that.

Test Plan

I've added a unit test in configuration making sure this runs successfully. I am unsure if this is sufficient so please let me know how I can improve the testing implemented in this PR.

@astral-sh-bot astral-sh-bot Bot added the ty Multi-file analysis & type inference label Apr 19, 2026
Comment thread crates/ty_server/src/session.rs Outdated
};

// Refresh the workspace with the new options.
self.workspaces.unregister(&root);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cleaning all workspaces just because e.g. the selected interpreter is a bit rough :)

Are there any settings that change the known workspaces? Is there a way we could only update the settings or is this much more complicated?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yea, initially I had considered some kind of diff checking to see what specific workspace settings had changed, but it seemed quite complicated between the pulled ClientOptions and the existing WorkspaceSettings in terms of implementation. If we could find a good solution for that we could:

A. Only update workspaces that we knew had changed
B. Only update the specific settings that had changed

Potentially this is done through manually comparing each setting on the two structs (and maybe returning or modifying the existing WorkspaceSettings)? Please let me know if you have any ideas 😃 .

Copy link
Copy Markdown
Author

@pharmac1st pharmac1st Apr 20, 2026

Choose a reason for hiding this comment

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

We could also just replace Arc<WorkspaceSettings> with the pulled settings as well, potentially we don't need to reregister and reinitialize unless the configuration file has changed? (This is the only thing I can see relying on settings in initialize_workspace_folder - https://github.com/pharmac1st/ruff/blob/a9ffadb41636a472c5a8a52aedca350ad64a8ac1/crates/ty_server/src/session.rs#L592).

We could also move handling updating global settings in update_workspace_folders, and not rely at all on initialize_workspace_folders.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd have to take a closer look, but just overriding the WorkspaceSettings seems fine; we don't need to diff them. But we also need to re-resolve the settings passed to the ProjectDatabase. It's okay if we replace the options if they don't compare equal, but that we should avoid is creating a new ProjectDatabase because that clears all caches

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated - I saw there was reload() which sort of did want we want, I split this function out into a more specific one that updates the settings.

I've also assumed that we want to update the global settings as well, and that should be set from the last workspace received (similar to how it's done in initialize_workspace_folders).

@carljm carljm removed their request for review April 20, 2026 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants