-
Notifications
You must be signed in to change notification settings - Fork 171
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
Enhance version manager config to accept more fields #1831
Conversation
031d839
to
853676a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any specific comments on the implementation but one configuration pattern that I really like and suggest we use is using shorthands. So, I suggest that we leave the rubyLsp.rubyVersionManager
config exactly as it is, but also allow it to be passed as an object if users want to pass in more options.
So, either "rubyLsp.rubyVersionManager": "chruby"
, or "rubyLsp.rubyVersionManager": { "identifier": "chruby", "foo": "bar" }
would be valid configuration, and I find that to be very user-friendly.
For reference, the older Ruby extension used that pattern for Ruby linter configuration: https://github.com/rubyide/vscode-ruby/blob/main/packages/vscode-ruby-client/package.json#L283-L343 You can see how |
So we'd avoid the
|
Talked with Ufuk on Slack. Unfortunately, if we use the In fact, you can add the enum and it does show the right completions, but then it considers the With the current implementation, old settings get migrated automatically and you get completion every step of the way, so I think this is a nicer approach. |
853676a
to
f391642
Compare
f391642
to
7be1827
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳 This is exciting!
This was missed in Shopify#1831
Motivation
Step towards #1822
Because each version manager has its own customizations, I believe we need to offer more settings to match the behaviour in the editor. We currently only allow setting the identifier of the manager, but that's not always enough (e.g.: see the issue).
Implementation
The idea is to change the
versionManager
setting so that it becomes an object and then we nest the identifier inside of that object. After this goes in, then we'll be able to add extra configuration fields, like the version manager executable path.All of the changes are solely to migrate
versionManager
from a string to an object that looks like{ identifier: "chruby" }
.The only other change is the automated migration in
extension.ts
, where we read all of the existing configuration values and update them automatically to match the new format. That code is temporary and can be removed after it has been there for a while.Automated Tests
Adapted our existing tests.
Manual Tests