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

Allow empty section string for configuration request #218

Merged

Conversation

ms
Copy link

@ms ms commented May 3, 2020

microsoft/vscode-eslint requests configuration with a section of ''.
This change returns the expected settings in that case.

https://github.com/microsoft/vscode-eslint/blob/be30e933c56ea6e8e579808d5f7c7b205c8e16cc/server/src/eslintServer.ts#L668

microsoft/vscode-eslint requests configuration with a section of ''.
This change returns the expected settings in that case.

https://github.com/microsoft/vscode-eslint/blob/be30e933c56ea6e8e579808d5f7c7b205c8e16cc/server/src/eslintServer.ts#L668
Copy link
Contributor

@h-michael h-michael left a comment

Choose a reason for hiding this comment

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

I think that if item.section doesn't include ".", VSCode does not return the whole config.settings but return config built by URI resource.

https://github.com/microsoft/vscode-languageserver-node/blob/082631da44d6799358602a4abebadbf4fc47859b/client/src/configuration.ts#L49-L51

@ms
Copy link
Author

ms commented May 10, 2020

So would you recommend leaving it as is? I'm struggling to find the code corresponding to workspace.getConfiguration() to see what it does (and whether the object it returns handles .get in any special way)

@h-michael
Copy link
Contributor

I'll check VSCode behavior.

@h-michael
Copy link
Contributor

@ms ms closed this Jun 13, 2020
@h-michael
Copy link
Contributor

@ms Sorry I haven't been able to start addressing this.
Once I have resolved some of the issues I will tackle this issue again.

@ms
Copy link
Author

ms commented Jun 15, 2020

No rush, Iike you point out in microsoft/language-server-protocol#972 it's not really clear what the client should do, and either way it's easy enough to add an empty string key for now.

@h-michael
Copy link
Contributor

@ms I read some clients implements and spec.
I think we can merge this changes.
Could you reopen this?

@ms ms reopened this Jun 16, 2020
@ms
Copy link
Author

ms commented Jun 16, 2020

Reopened, feel free to merge as is or push changes if you'd like

@h-michael h-michael merged commit a82ce0e into neovim:master Jun 16, 2020
@h-michael
Copy link
Contributor

@ms ty : )

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