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 plugin overwrite on_workspace_configuration #2132

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

LDAP
Copy link
Contributor

@LDAP LDAP commented Dec 8, 2022

Currently, plugins can not set the configuration object in on_workspace_configuration if it is None or it has to be replaced completely.

This did only work in those cases where configuration was a reference to an object that could be altered.

See
sublimelsp/LSP-jdtls@85f2392
for an example where the current implementation does not work.

This PR changes on_workspace_configuration so that it must return the new configuration item. The default implementation now forwards the "pre-resolved" configuration.

@rchl
Copy link
Member

rchl commented Dec 8, 2022

It's a breaking change for LSP-eslint: https://github.com/sublimelsp/LSP-eslint/blob/fd903029a075497d6c8eeecbe980a6c4748e6ac7/plugin.py#L70-L70

Will you also create a PR that adapts it to use the changed API?

(Normally I would prefer a non-breaking change or new API but I think that we can pretty confidently say that nothing else is using this API so it should be fine.)

@LDAP
Copy link
Contributor Author

LDAP commented Dec 8, 2022

Yes, I'll create a PR there too.

@LDAP
Copy link
Contributor Author

LDAP commented Dec 8, 2022

I opened a PR as far as I can see we can publish the LSP-eslint change without breaking anything. The LSP must be released before LSP-jdtls.

(I tried to think of a non-breaking solution but could not come up with something better)

LDAP added a commit to LDAP/LSP-eslint that referenced this pull request Dec 9, 2022
LDAP added a commit to LDAP/LSP-eslint that referenced this pull request Dec 9, 2022
rchl pushed a commit to sublimelsp/LSP-eslint that referenced this pull request Dec 9, 2022
@rchl rchl merged commit c5c2cf0 into sublimelsp:main Dec 9, 2022
@rchl
Copy link
Member

rchl commented Dec 30, 2022

Released new LSP version with this fix.

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.

2 participants