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

Fix Partial Configuration Bug #258

Merged

Conversation

trajamsmith
Copy link
Contributor

Small patch to fix #256.

When submitting partial settings objects to the language server using the onDidChangeConfiguration listener, the server was overwriting its default values with null values for every unspecified parameter.

@coveralls
Copy link

coveralls commented May 1, 2020

Coverage Status

Coverage decreased (-0.2%) to 77.042% when pulling 068e925 on Trajamsmith:partial-configurations into 27522e8 on redhat-developer:master.

yamlConfigurationSettings = settings.yaml.schemas;
yamlShouldValidate = settings.yaml.validate;
yamlShouldHover = settings.yaml.hover;
yamlShouldCompletion = settings.yaml.completion;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were overwritten if, e.g., yaml.validate.enabled was not specified in the configuration message.

@JPinkney
Copy link
Contributor

JPinkney commented May 1, 2020

Hmm after looking at it again I think there might be a small bug here:
ex for yamlShouldValidate

if settings.yaml.validate is undefined it will take the value of yamlShouldValidate ✔️
if settings.yaml.validate is true it will take that value ✔️

but
I think if settings.yaml.validate is false it will take the value of yamlShouldValidate if that's true, leading it to not change

I think we might have to do something like settings.yaml.validate ? settings.yaml.validate : yamlShouldValidate

That way of settings.yaml.validate is undefined it uses yamlShouldValidate
If settings.yaml.validate is defined and true it will use it
If settings.yaml.validate is defined and false it will use it

@trajamsmith
Copy link
Contributor Author

You're absolutely right, I got the configuration object confused with that of another language server 😅 I think those ternary operators would still cause a problem though — if settings.yaml.validate is false (we want to set it to false) and yamlShouldValidate is true, then it will still be true.

What about just checking for the property:

if (settings.yaml.hasOwnProperty('validate') {
  yamlShouldValidate = settings.yaml.validate
}

@JPinkney
Copy link
Contributor

JPinkney commented May 1, 2020

if settings.yaml.validate is false (we want to set it to false) and yamlShouldValidate is true, then it will still be true.

Oh right 😅

I think just checking the property should suffice then!

In that case, if its undefined then nothing changes and if it is defined then it changes to whatever it was defined as

@trajamsmith
Copy link
Contributor Author

Alright, it's a little more verbose but I feel like that should work 👍

Copy link
Contributor

@JPinkney JPinkney left a comment

Choose a reason for hiding this comment

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

Looks good from my side 👍

@JPinkney JPinkney merged commit 814e43a into redhat-developer:master May 1, 2020
@trajamsmith
Copy link
Contributor Author

Do you all have an idea of when you might next publish an NPM release? Would be great to have these configs available sooner than later, but it's not a big deal either way. Thanks for letting me help!

@JPinkney
Copy link
Contributor

JPinkney commented May 1, 2020

Hopefully in the next week or so, there's a decent amount of stuff that needs to be released now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Defaults for didChangeConfiguration Messages
3 participants