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

Introduce .ruby-lsp.yml config file #1434

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Feb 29, 2024

Motivation

We want to have a general purpose configuration mechanism for Ruby LSP that not tied to a particular editor.

Also closes #1295 (but with a slightly different approach)

Implementation

As agreed with @vinistock, we will repurpose the current indexing configuration file, .index.yml.

There will be a new top-level key, indexing, but within that the structure was the same as before.

Automated Tests

Included

Manual Tests

  • Check out this branch
  • Ensure indexing is working (e.g. the Jump to Definition works for constants)
  • Edit .ruby-lsp.yml and rename the indexing key.
  • Restart the LSP and verify that error notification appears (the remaining LSP feature should continue working)
  • Copy .index.yml from main, and delete .ruby-lsp.yml.
  • Restart the LSP and ensure a deprecation warning about .index.yml shows in the output panel.
  • Ensure an error message is showing but otherwise everything works (e.g. formatting)
  • Delete .index.yml and restart the LSP (so now there are no config files)
  • Ensure indexing completes successfully (e.g. the Jump to Definition works for constants)

@andyw8 andyw8 force-pushed the andyw8/introduce-config-file branch 3 times, most recently from 8aeeaa7 to 762cc52 Compare February 29, 2024 15:56
@andyw8 andyw8 added the enhancement New feature or request label Feb 29, 2024
@andyw8 andyw8 force-pushed the andyw8/introduce-config-file branch from 762cc52 to 16480f7 Compare February 29, 2024 16:03
@andyw8 andyw8 force-pushed the andyw8/introduce-config-file branch 5 times, most recently from 098a109 to ad1b277 Compare February 29, 2024 21:15
@andyw8 andyw8 force-pushed the andyw8/introduce-config-file branch from ad1b277 to 2378bda Compare March 22, 2024 16:47
@andyw8 andyw8 changed the base branch from main to andyw8/pass-config-into-indexer-v2 March 22, 2024 16:48
@andyw8 andyw8 force-pushed the andyw8/introduce-config-file branch 3 times, most recently from 6e67b3d to e0b729e Compare March 22, 2024 17:41
ruby_lsp_path = File.join(@workspace_uri.to_standardized_path, ".ruby-lsp.yml")

if File.exist?(index_path)
$stderr.puts("The .index.yml configuration file is deprecated. Please rename it to .ruby-lsp.yml and update the
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm wondering if this should display a notification.

@andyw8 andyw8 added the server This pull request should be included in the server gem's release notes label Mar 22, 2024
@andyw8 andyw8 marked this pull request as ready for review March 22, 2024 17:45
@andyw8 andyw8 changed the title WIP: Introduce .ruby-lsp.yml config file Introduce .ruby-lsp.yml config file Mar 22, 2024
@andyw8 andyw8 requested a review from vinistock March 22, 2024 17:45
@andyw8 andyw8 requested a review from a team as a code owner March 22, 2024 17:46
@andyw8 andyw8 requested a review from st0012 March 22, 2024 17:46
@andyw8
Copy link
Contributor Author

andyw8 commented Mar 22, 2024

(looks like some tests need a slightl adjustment for Windows)

Base automatically changed from andyw8/pass-config-into-indexer-v2 to main March 25, 2024 18:08
@andyw8 andyw8 force-pushed the andyw8/introduce-config-file branch from e0b729e to 91fefb7 Compare March 25, 2024 18:16
@andyw8 andyw8 force-pushed the andyw8/introduce-config-file branch from 91fefb7 to 54aa985 Compare March 25, 2024 18:28
@vinistock
Copy link
Member

I think the changes are good, but I wonder if having the configuration file isn't deviating from the LSP approach. We might be better off getting rid of the .index.yml (with a deprecation warning first) and then start accepting all of the configuration from the editor.

After #1818 is shipped, we could have the indexing configuration be a part of the global state and gather the configuration from the initialization options.

I think it's worth ensuring we're following the LSP approach at this point, because otherwise we will have introduced two configuration files (.index.yml and .ruby-lsp.yml), both of which may be not following the standard.

Thoughts?

@andyw8
Copy link
Contributor Author

andyw8 commented Mar 25, 2024

TODO: update ruby-lsp-doctor to handle new config

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
2 participants