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

Read values for workspace/configuration request from settings #908

Merged
merged 4 commits into from
Mar 16, 2020

Conversation

jwortmann
Copy link
Member

The workspace/configuration request seems to be implemented differently in various servers:

  • The Julia server sends multiple configuration keys as section
  • The ESLint server sends a blank section instead, but expects a single dict of configuration settings.
  • According to Support workspace/configuration #737 (comment) the Kotlin server sends a single "kotlin" as section.

This PR makes LSP to read each section from the server's settings in LSP.sublime-settings, so for the Julia server they could for example be

"settings": {
    "julia.format.calls": false,
    "julia.format.comments": false,
    "julia.format.curly": false,
    "julia.format.docs": false,
    "julia.format.indent": 4,
    "julia.format.indents": false,
    "julia.format.iterOps": false,
    "julia.format.kw": false,
    "julia.format.lineends": false,
    "julia.format.ops": false,
    "julia.format.tuples": false,
    "julia.lint.call": false,
    "julia.lint.constif": false,
    "julia.lint.datadecl": false,
    "julia.lint.iter": false,
    "julia.lint.lazy": false,
    "julia.lint.modname": false,
    "julia.lint.nothingcomp": false,
    "julia.lint.pirates": false,
    "julia.lint.run": false,
    "julia.lint.typeparam": false
},

If a 'section' name is missing in the settings, LSP should use None as its value, as described in https://microsoft.github.io/language-server-protocol/specification#workspace_configuration.
A separate fix is needed for the ESLint server, because its 'section' value is blank.

The Kotlin server would require

"settings": {
    "kotlin": {
        // specific settings here
    }
}

This might require a change for users who use this server, but I think it is better than adding another special case just for that server. I have not tested with the Kotlin server though, perhaps someone could test if it works before merging?

The scopeURI described in the specs is still ignored here by LSP (it is not used in the Julia server).

Fixes #907

Related: #699

See: https://microsoft.github.io/language-server-protocol/specification#workspace_configuration

The Julia server sends multiple configuration keys as 'section'.
This commit extracts the corresponding values for those keys from the
"settings" entry of the server configuration, and returns None for
missing values, as described in the protocol specification.

A special case is needed for the ESLint server, which sends a blank
section instead, but expects a dict of configuration settings.
@jwortmann jwortmann force-pushed the workspace-configuration branch from 936bb35 to 31681f1 Compare March 13, 2020 21:57
@rwols
Copy link
Member

rwols commented Mar 14, 2020

Can we split the requested configuration key on the dot, and then go into the settings dictionary for each token?

>>> 'julia.format.calls'.split('.')
['julia', 'format', 'calls']

So something like this:

def get_value_from_section(current: Any, section: str) -> Any:
    keys = section.split('.')
    for key in keys:
        if isinstance(current, dict):
            current = current.get(key)
        else:
            return None
    return current


for requested_item in requested_items:
    section = requested_item['section']
    items.append(get_value_from_section(self.config.settings, section))

I hope this'll work with these kinds of settings, more in line with kotlin and eslint:

"settings": {
    "julia": {
        "format": {
            "calls": false,
            "comments": false
        },
        "lint": {
            "call": false,
            "modname": false
        }
    }
}

plugin/core/sessions.py Outdated Show resolved Hide resolved
@jfcherng
Copy link
Contributor

jfcherng commented Mar 14, 2020

Can we split the requested configuration key on the dot, and then go into the settings dictionary for each token?

I have something like that in my plugin too! It's really handy.

https://github.com/jfcherng/Sublime-OpenUri/blob/55c301b7f4f9b19d412cbd1b0e13a3aa017bbc8c/plugin/utils.py#L23-L47

@tomv564
Copy link
Contributor

tomv564 commented Mar 14, 2020

To be honest I think the Julia server's choice of requesting individual values using this parameter (it's called section for a reason) is a bit special and I personally wouldn't go out of my way to accommodate it. Users can just put eg "julia.format.calls" in their LSP settings, it's ugly but doesn't cost us any extra logic.

@jwortmann
Copy link
Member Author

To be honest I think the Julia server's choice of requesting individual values using this parameter (it's called section for a reason) is a bit special and I personally wouldn't go out of my way to accommodate it. Users can just put eg "julia.format.calls" in their LSP settings, it's ugly but doesn't cost us any extra logic.

I've checked how VS Code handles its settings for the Julia server: they also just put single dotted strings e.g. "julia.format.calls" in their settings and don't split on dots, and personally I would prefer a flat settings structure as well. However, if we use nested setting structures elsewhere, I'm also fine with adding a function to split requested values into separate tokens, if requested for consistency.

In my opinion, neither the Julia server nor ESLint use section as intended in the specs:

The configuration section ask for is defined by the server and doesn’t necessarily need to correspond to the configuration store used be the client. So a server might ask for a configuration cpp.formatterOptions but the client stores the configuration in a XML store layout differently. It is up to the client to do the necessary conversion.

So for the Julia server implementation it would make more sense to ask for two objects 'julia.format' and 'julia.lint', so our setting structure would be

"settings": {
    "julia.format": {
        "calls": false,
        "comments": false,
        "curly": false
    },
    "julia.lint": {
        "call": false,
        "constif": false,
        "datadecl": false
    }
}

If the way like how ESLint just expects a single nested settings structure was the intended way, why would it have been wrapped into an array with different ConfigurationItems then in the specifications?

@rchl
Copy link
Member

rchl commented Mar 14, 2020

The Kotlin server would require

You can update docs/index.md with that information

@rwols
Copy link
Member

rwols commented Mar 14, 2020

So what happens if one day julia-ls decides to request a single julia section?

If we decide to allow foo.bar.baz-style settings, we should translate it into a dictionary if a server requests foo, no?

{
  "foo.bar.baz": 1,
  "foo.bar.qux": "frobnicate"
}

If a server requests foo.bar.baz and foo.bar.qux, then we return [1, 'frobnicate']. OK.
Now the server has an update and it requests foo.bar. Now we return [None]. We should instead return [{'baz': 1, 'qux': 'frobnicate'}]. It just feels like a major omission to me.

And the other way around also seems broken.
e.g. if kotlin starts requesting kotlin.formatting.tabSize and kotlin.formatting.translateTabsToSpaces then we'll return [None, None].

The difference in settings-structure furthermore makes it difficult for new users to understand what the general rule about these settings are.

@jwortmann
Copy link
Member Author

jwortmann commented Mar 14, 2020

So what happens if one day julia-ls decides to request a single julia section?

I see that is a goood point and would add flexibility for such kind of request changes from servers.

But what if the server decides to request julia, but expects [{ "format.call": true, "format.comments": true, "lint.call": true }] etc.
In general, if we split incoming requests on dots, there is no way to access dotted keys in the settings, as we would only look for the first part of the key name.

Edit: OK I think that objection didn't make sense 🙈
I guess it is indeed advantageous to have a dict structure of settings and split requests on dots.

Copy link
Member

@rwols rwols left a comment

Choose a reason for hiding this comment

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

LGTM. I'm happy all settings structures/schemas for all servers are the same.

@rwols rwols merged commit c7301fb into sublimelsp:master Mar 16, 2020
@jwortmann jwortmann deleted the workspace-configuration branch March 16, 2020 17:58
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.

Send null when a setting doesn't match in workspace/configuration request
5 participants