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

Defaults for didChangeConfiguration Messages #256

Closed
trajamsmith opened this issue Apr 30, 2020 · 3 comments · Fixed by #258
Closed

Defaults for didChangeConfiguration Messages #256

trajamsmith opened this issue Apr 30, 2020 · 3 comments · Fixed by #258
Labels

Comments

@trajamsmith
Copy link
Contributor

Hey — we're trying to extend the functionality of JupyterLab to include all the features that language servers provide. At present, we're taking a look at server configuration messages. Right now, our onDidChangeConfiguration messages to a few server implementations are working well, but we're having small issues with the yaml-language-server.

We want our settings objects in the JupyterLab editor look something like this:

{
    "language_servers": {
        "pyls": {
            "serverSettings": {
                "pyls.plugins.pydocstyle.enabled": true,
                "pyls.plugins.pyflakes.enabled": false,
                "pyls.plugins.flake8.enabled": true
            }
        },
        "r-languageserver": {
            "serverSettings": {
                "r.lsp.debug": true,
                "r.lsp.diagnostics": false
            }
        },
        "yaml-language-server": {
            "serverSettings": {
                "yaml.format.enable": true,
                "yaml.format.singleQuote": true
            }
        }
    }
}

We'd like to be able to send partial configurations like the above, but I believe the issue we're having is that the onDidChangeConfiguration listener for the YAML server doesn't like partial configurations.

My best guess is that this is because there are no defaults here:

    if (settings.yaml) {
        yamlConfigurationSettings = settings.yaml.schemas;
        yamlShouldValidate = settings.yaml.validate;
        yamlShouldHover = settings.yaml.hover;
        yamlShouldCompletion = settings.yaml.completion;
        customTags = settings.yaml.customTags ? settings.yaml.customTags : [];

        if (settings.yaml.schemaStore) {
            schemaStoreEnabled = settings.yaml.schemaStore.enable;
        }
...

So if we send the configuration settings like I've written them up top, then the server turns off validation, even though my configuration object didn't mention validation.

Is that hunch correct? It's very possible that we're just looking at this the wrong way.

@trajamsmith trajamsmith changed the title Defaults for didChangeConfiguration Message Defaults for didChangeConfiguration Messages Apr 30, 2020
@JPinkney
Copy link
Contributor

Yeah it looks like that's correct, partial configurations don't seem to be working correctly

@JPinkney JPinkney added the bug label Apr 30, 2020
@trajamsmith
Copy link
Contributor Author

May I submit a PR for this? This fixes the issue on my local:

    // change this
    if (settings.yaml) {
        yamlConfigurationSettings = settings.yaml.schemas;
        yamlShouldValidate = settings.yaml.validate;
        yamlShouldHover = settings.yaml.hover;
        yamlShouldCompletion = settings.yaml.completion;
        customTags = settings.yaml.customTags ? settings.yaml.customTags : [];

    // to this
    if (settings.yaml) {
        yamlConfigurationSettings = settings.yaml.schemas || yamlConfigurationSettings;
        yamlShouldValidate = settings.yaml.validate || yamlShouldValidate;
        yamlShouldHover = settings.yaml.hover || yamlShouldHover;
        yamlShouldCompletion = settings.yaml.completion || yamlShouldCompletion;
        customTags = settings.yaml.customTags ? settings.yaml.customTags : [];

@JPinkney
Copy link
Contributor

JPinkney commented May 1, 2020

Yeah! A PR would be great!

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

Successfully merging a pull request may close this issue.

2 participants