Don't ignore incomplete server config, use default values if needed#432
Don't ignore incomplete server config, use default values if needed#432nrc merged 1 commit intorust-lang:masterfrom
Conversation
| use serde::de::Error; | ||
|
|
||
| trace!("config change: {:?}", params.settings); | ||
| let config = params.settings.get("rust") |
There was a problem hiding this comment.
Changed to chaining result to get better error messages (which incidentally lead me to the cause of the problem). Is this okay?
| trace!("config change: {:?}", params.settings); | ||
| let config = params.settings.get("rust") | ||
| .ok_or(serde_json::Error::missing_field("rust")) | ||
| .and_then(|value| Config::deserialize(value)); |
There was a problem hiding this comment.
Asked over at #serde and changed to Type::serialize, since from what I understand it allows to deserialize from ref directly instead of cloning the Value beforehand
| ).unwrap(); | ||
| out.response(output); | ||
| } | ||
| use serde::Deserialize; |
There was a problem hiding this comment.
Are those okay in the fn scope or should I move them to the top of the file?
There was a problem hiding this comment.
I'd move them to the top. I'm not really a fan of function-scope use, except in cases where you would never want the imports elsewhere in the module (as opposed to just happening not to need them right now)
nrc
left a comment
There was a problem hiding this comment.
Looks good, only thing to change is moving the imports out of the function
| ).unwrap(); | ||
| out.response(output); | ||
| } | ||
| use serde::Deserialize; |
There was a problem hiding this comment.
I'd move them to the top. I'm not really a fan of function-scope use, except in cases where you would never want the imports elsewhere in the module (as opposed to just happening not to need them right now)
| use serde::de::Error; | ||
|
|
||
| trace!("config change: {:?}", params.settings); | ||
| let config = params.settings.get("rust") |
| trace!("config change: {:?}", params.settings); | ||
| let config = params.settings.get("rust") | ||
| .ok_or(serde_json::Error::missing_field("rust")) | ||
| .and_then(|value| Config::deserialize(value)); |
|
Moved the imports out and pushed the new version |
This probably fixes #431 and related.
In the current implementation serialization for RLS options can fail if not every option is provided, and if that happens, configuration is essentially ignored. Since for some time few options weren't included in the VS Code (and possibly ignored completely when using different clients) extension, user configuration had no effect.
This makes it so that missing options are set to the values set in
Defaultimpl forConfig.