[rchl]

In a3bd1df the ClientConfig equality check was simplified to checking _all_settings but that's not correct in case someone changes config.initialization_options (or any other explicitly exposed property) after config was created because _all_settings wouldn't get updated in that case.

In practice this was maybe not a problem (or not always a problem) because before checking for equality the new config would be re-created from latest sublime settings so both _all_settings and initialization_options would change in sync.

[jwortmann]

The first part name of this addition seems redundant if it's also checked below, and the the second part _all_settings feels wrong if we want to define __eq__ as a "dynamic equality". I'd say we should either decide for "dynamic" equality, i.e. later changes to the fields are considered (the change from this PR), or for "static" equality, which considers only the config from the JSON (sublime-settings) file. But with this change, ClientConfigs are only equal if they satisfy both conditions, which doesn't feel right to me.

[rchl]

Removed redundant name check.

As for _all_settings it's really intended to only contain properties that are not already explicitly exposed on ClientConfig.

[rchl]

I admit that it's a bit confusing now how it works - the _all_settings contains the original settings but those can drift out of sync with explicitly exposed ClientConfig properties that have the same name. The getter implementation ensured that those shadowed properties can't be accessed on _all_settings but since I've transitioned to root_settings, that's no longer the case.

I'm not sure what would be the best way forward. Current way works only that it can be confusing when it comes to the value of those shadowed keys. Perhaps we should explicitly remove those from _all_settings at least?

[jwortmann]

_all_settings contains entire JSON dict from the settings file, except for the "settings" key:

LSP/plugin/core/types.py

Line 971 in de4034e

all_settings=deepcopy(s.to_dict())

LSP/plugin/core/types.py

Lines 859 to 862 in de4034e

	self._all_settings = all_settings
	# Exclude 'settings' because it shouldn't be considered when ClientConfig is checked for equality
	if 'settings' in self._all_settings:
	del self._all_settings['settings']

So it compares static "command" / "initialization_options", etc. from the settings file too. So IMO that check should be removed, if we also want to compare the same things with dynamic values.

Btw, that last 2 lines (and comment) aren't necessary anymore after removing that check.

[jwortmann]

An alternative would be to keep that check, but remove all known keys from _all_settings. Then config would still be restarted when custom settings change.

[rchl]

Yes, that was my thinking too.

Though we don't really have programmatically defined list of root properties of sublime-settings config. For example ClientConfig has name property but it should still be allow on all_settings because it's not treated in any special way in sublime-settings.

[rchl]

508b457
plus rewording:
0cff53b
16f70cf

[jwortmann]

I didn't see your previous comment until page reload. Ok, I would probably at least remove command and initialization_options from _all_settings.

I think the two lines I used above could be simplified to self._all_settings.pop('settings', None).

[rchl]

And now you also didn't see my change before posting. :) We are too fast.

[jwortmann]

By the way, is there any good use case for allowing to modify config.initialization_options? If not, in my opinion this should not be / have been exposed this way as a mutable property. For example, LSP-jdtls for some reason overwrites and completely ignores the user's "initialization_options" from the .sublime-settings file, which did cost me some time debugging while trying to make that server work. https://github.com/sublimelsp/LSP-jdtls/blob/2e675dc8a3a9442171777323d0a42a2e5ee24475/modules/jdtls.py#L177

[rchl]

By the way, is there any good use case for allowing to modify config.initialization_options? If not, in my opinion this should not be / have been exposed this way as a mutable property. For example, LSP-jdtls for some reason overwrites and completely ignores the user's "initialization_options"

There are good reasons to mutate initialization_options (or other keys) dynamically. One could ask a question whether it should be done by mutating ClientConfig or by overriding request but I guess it doesn't really make a big difference for this case.

For example LSP-typescript enables plugins dynamically based on plugins it finds in the plugin host.

If LSP-jdtls does something weird then its up to it to do better.

[rchl]

Missed those 3 because those are not documented in https://lsp.sublimetext.io/client_configuration/

[jwortmann]

I think we have broke the "enabled": true/false toggle in configs.

[rchl]

Will look. Do you understand why?

[rchl]

I see, because it's _enabled.

[jwortmann]

Yes, so we either need to keep it in _all_settings, i.e. remove from CONFIG_KEYS, or it needs special handling later in __eq__.

[rchl]

#2940 - decided to use CONFIG_KEYS as a source of truth (and exclude settings).