-
Notifications
You must be signed in to change notification settings - Fork 98
feat: allow configuration through workspace/didChangeConfiguration #316
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?
feat: allow configuration through workspace/didChangeConfiguration #316
Conversation
This is literally my first time writing Rust, ever. lmao. Furthermore, I started this in draft because I'm not sure what you want with regards to testing and such. Is it safe to change workspace settings at runtime? No idea. Is my code nasty? Certainly. Anyways, let me know how/if you'd like to proceed with this. I thought it would be pretty straightforward after thinking it through while reading #302 and, I don't know, kinda felt the random urge to finally give Rust a try :) |
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.
Hey hey! 👋
First of all, thank you for the contribution :) And sorry for the late reply.
The approach looks good to me; I first thought that it would be nicer to use a update_settings
method for this, but I think it's fine as it is.
I have a few remarks regarding code, but great first Rust PR 👍
@juleswritescode 166f5aa addresses your comments, or at least attempts to :D |
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 think it looks good 👍🏻👍🏻
Heads up, don't worry about the linting issues, this pr fixes that. Once that's merged, could you run and run ![]() |
166f5aa
to
550049e
Compare
@@ -1,5 +1,5 @@ | |||
{ | |||
"$schema": "https://biomejs.dev/schemas/1.9.4/schema.json", | |||
"$schema": "./node_modules/@biomejs/biome/configuration_schema.json", |
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.
so you don't have to update the schema url if you upgrade biome
@@ -8,7 +8,7 @@ | |||
"files": { | |||
"ignoreUnknown": false, | |||
"ignore": [], | |||
"include": ["packages/**/*"] | |||
"include": ["/packages/**/*"] |
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.
only files in ./packages (the leading "/" has the same meaning as in gitignore(5))
@juleswritescode I made a minor change to your biome.jsonc, see inline comments |
What kind of change does this PR introduce?
It enables passing workspace settings via the workspace/didChangeConfiguration notification. This in turn enables the client to specify settings dynamically, rather than being limited to configuration files. This is a solution to #302. See example usage (with lspconfig) here: willruggiano/neovim.drv@9aa06ad.
What is the current behavior?
There is none. The payload of this handler is currently ignored.
What is the new behavior?
The configuration received by the handler is merged with the fs configuration.