Skip to content
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

Add watched_files_dynamic_registration to global state #1905

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

Earlopain
Copy link
Contributor

Motivation

Work towards Shopify/ruby-lsp-rails#326. ruby-lsp-rails wants to watch for file changes to **/structure.sql. This allows it to check if the client supports this functionality before trying to send the request.

It makes sense to add docs on how/why you might make use of this option in an addon, but I'll defer that until after I've actually written some functioning code.

Implementation

Move the options extraction for this into the global state.

Automated Tests

Add a few variants that a client might send.

Manual Tests

Edit a ruby file and observe that the file watcher is still being registered, i.e. that the index is being updated.

@Earlopain Earlopain requested a review from a team as a code owner April 9, 2024 19:18
@Earlopain Earlopain requested review from Morriar and KaanOzkan April 9, 2024 19:18
@KaanOzkan KaanOzkan added enhancement New feature or request server This pull request should be included in the server gem's release notes labels Apr 10, 2024
@KaanOzkan KaanOzkan requested a review from a team April 10, 2024 14:32
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

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

I have a minor nitpick, but this looks great!

lib/ruby_lsp/global_state.rb Outdated Show resolved Hide resolved
This allows addons to check if they can register custom file change listerens. Work towards Shopify#326

It makes sense to add docs on how to make use of this option,
but I'll defer that until after I've actually written some functioning code with it.
@Earlopain Earlopain force-pushed the global-state-dynamic-watchers branch from 4347f63 to 430bb6c Compare April 10, 2024 16:28
@vinistock
Copy link
Member

Thank you for the contribution!

@vinistock vinistock merged commit f98f84f into Shopify:main Apr 10, 2024
21 checks passed
@Earlopain Earlopain deleted the global-state-dynamic-watchers branch April 10, 2024 16:45
@Earlopain
Copy link
Contributor Author

Thanks for the merge. What would be the steps to make use of this in ruby-lsp-rails? I believe I would need to wait for a 0.17 release and it to be adapted there, or is a minimal version requirement bump to a theoretical 0.16.4 acceptable?

@vinistock
Copy link
Member

A v0.16.4 would be fine. We're using minor versions for breaking changes at the moment, since the LSP is under a lot of work (to avoid having so many major bumps until the project stabilizes).

We can bump the ruby-lsp-rails requirement to v0.16.4 and make use of the new variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants