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 fetching nested http settings #511

Merged
merged 2 commits into from
Jul 19, 2021
Merged

Fix fetching nested http settings #511

merged 2 commits into from
Jul 19, 2021

Conversation

robb-j
Copy link
Contributor

@robb-j robb-j commented Jul 15, 2021

What does this PR do?

This PR tweaks the way the http configuration section is pulled from the client. It pulls the entire http section at once then plucks out the values it needs. I do assume there is no sensitive client information other that proxy and proxyStrictSSL on the http section which shouldn't be pulled.

In my research I've found no solid definition for what a section in the LSP is so my thinking is that usage of it should be as simple as possible for most clients to support it.

What issues does this PR fix or reference?

Nova doesn't currently have a way to configure how workspace settings are passed to the server and it does it in a bit of an odd way at the moment, which I've described in the issue below. Requesting http.proxy in Nova returns {} which was getting passed straight to requests_light and breaking it.

Is it tested? How?

With unit tests. I updated the existing unit test to follow the new LSP message structure and added an assertion for the shape of settings passed to setConfiguration.

I ran the modified server in Nova using npm link and confirmed that schemas were being loaded and tooltips were showing again.

I can try running it with VSCode if you like but I don't currently have an extension development setup for it.

@evidolob
Copy link
Collaborator

@robb-j Could you fix build?

@coveralls
Copy link

coveralls commented Jul 16, 2021

Coverage Status

Coverage decreased (-0.01%) to 78.144% when pulling 82de081 on robb-j:main into e44fc1b on redhat-developer:main.

@robb-j
Copy link
Contributor Author

robb-j commented Jul 16, 2021

That's fixed, it was a code formatting issue in the test.

@evidolob evidolob merged commit 9042999 into redhat-developer:main Jul 19, 2021
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.

4 participants