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

Language server configuration management #119

Merged
merged 16 commits into from
Sep 29, 2017
Merged

Conversation

tomv564
Copy link
Contributor

@tomv564 tomv564 commented Sep 20, 2017

Onboarding flow

Notification when a supported syntax is detected:

screen shot 2017-09-20 at 21 46 06

Documentation shown when running "Setup Language Server":

screen shot 2017-09-20 at 21 46 48

Clicking one of the Enable options in the bottom will copy the default config to clients in your user settings and set the enabled flags at the user/project level if needed. LSP starts up immediately.

Copying global config vs. merging from default

The "clients" setting in the User LSP.sublime-settings fully replaced the defaults, causing #76 and #117.
Moving the default clients to their own setting key fixes #76 (as there are no default-enabled clients, you don't lose any by defining "clients" in user settings)
As we copy the entire config of a client into user settings, #117 will still not be solved: There will be no way to "use the default config, and only override the command field"
Is this feature worth dedicating even more lines of config-wrangling code to?

TO DO

  • Determine if merging config up from default_clients is worthwhile (User settings are all-or-nothing #117)
  • Better naming and handling of unparsed settings and parsed configs
  • Remove duplication around project_data handling
  • Evaluate impact to users with existing settings
  • Find a better way to show documentation - the popup looks terrible, just opening the MD is likely better.
  • Update documentation structure so we can open/link to individual language setups

@deathaxe
Copy link
Contributor

deathaxe commented Sep 21, 2017

As we copy the entire config of a client into user settings, #117 will still not be solved: There will be no way to "use the default config, and only override the command field"

There is a way to do so, I think. You could try loading the settings by the following two lines:

clients = settings.get("default_clients")
clients.update(settings.get("user_clients"))

clients.update() merges the two configurations basing on the default_clients.

By modifying the configuration structure as array, these would be the only two lines needed to read in the client configuration - except you'd like to do some value checks.

@tomv564
Copy link
Contributor Author

tomv564 commented Sep 27, 2017

@deathaxe I looked into this today, this should work if we update each client config. Thanks for the tip!

@tomv564
Copy link
Contributor Author

tomv564 commented Sep 27, 2017

The user benefit of merging up from defaults instead of copying defaults is that if LSP improves the defaults (eg. extra syntaxes or scopes) the user will immediately get those updates.
The downside is that every such update is potentially a breaking update (esp. when it comes to command). But this will probably not happen too often.

supported_client_configs use the settings in default_clients

global_client_configs inherit the settings from default_clients

In order to enable eg. pyls globally, the User LSP.sublime-settings should have:

"clients": {
  "pyls": {
    "enabled": true
  }
}

project_client_configs inherit the settings from clients and default_clients

In order to enable pyls, a sublime-project should have:

"settings": {
  "LSP": {
    "clients": {
      "pyls": {
        "enabled": true
      }
    }
  }
}

@deathaxe
Copy link
Contributor

The downside is that every such update is potentially a breaking update (esp. when it comes to command). But this will probably not happen too often.

If a user overrides a setting, he must ensure his changes work. You could probably do some format/type checks against the user configuration before merging. If a key is present, it must match the desired type. On the other hand, if things are merged and something goes wrong, because of invalid settings, you can be quite sure its the user settings issue and output a descriptive error.

I find the idea of merging things together better than ask the user to copy the whole structure. Can be compared with patching a color scheme vs. a theme. Later one is much easier to adjust as you just need to create a file with few lines to patch the original one.

@tomv564 tomv564 merged commit 9a54656 into master Sep 29, 2017
@tomv564 tomv564 deleted the feature-config-management branch October 8, 2017 19:44
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.

[Feature request] Merge configurations?
2 participants