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

Custom handlers not override built in handlers correctly #392

Merged
merged 8 commits into from Oct 21, 2020

Conversation

david-driscoll
Copy link
Member

@david-driscoll david-driscoll commented Oct 13, 2020

  • Fixed a bug in configuration (configuration values were case-sensitive).
  • Added unit tests demonstrating Configuration.Binder, and Options usages

@david-driscoll david-driscoll added the bug Something isn't working label Oct 13, 2020
@github-actions github-actions bot added this to the v0.18.1 milestone Oct 13, 2020
@david-driscoll david-driscoll added enhancement New feature or request and removed bug Something isn't working labels Oct 13, 2020
@codecov
Copy link

codecov bot commented Oct 13, 2020

Codecov Report

Merging #392 into master will increase coverage by 0.00%.
The diff coverage is 68.85%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #392   +/-   ##
=======================================
  Coverage   75.83%   75.84%           
=======================================
  Files         561      561           
  Lines       13511    13536   +25     
  Branches     1247     1247           
=======================================
+ Hits        10246    10266   +20     
- Misses       3265     3270    +5     
Impacted Files Coverage Δ
src/Client/LanguageClientOptions.cs 35.55% <0.00%> (ø)
...nRpc.Generators/GenerateHandlerMethodsGenerator.cs 84.24% <ø> (ø)
src/Protocol/LanguageProtocolDelegatingHandlers.cs 40.83% <0.00%> (-0.74%) ⬇️
...otocol/Workspace/IDidChangeConfigurationHandler.cs 0.00% <0.00%> (ø)
...col/Workspace/IDidChangeWorkspaceFoldersHandler.cs 0.00% <ø> (ø)
src/Server/DefaultLanguageServerFacade.cs 82.35% <0.00%> (ø)
src/Server/LanguageServerOptions.cs 23.52% <0.00%> (ø)
...sonRpc/JsonRpcServerServiceCollectionExtensions.cs 88.81% <72.00%> (-4.53%) ⬇️
...lient/LanguageClientServiceCollectionExtensions.cs 96.93% <100.00%> (ø)
...er/Configuration/DidChangeConfigurationProvider.cs 87.91% <100.00%> (+0.67%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b88655...ca564c0. Read the comment docs.

@david-driscoll david-driscoll added bug Something isn't working and removed enhancement New feature or request labels Oct 15, 2020
@david-driscoll david-driscoll changed the title Configuration binding and options Custom handlers not override built in handlers correctly Oct 15, 2020
Copy link

@ajaybhargavb ajaybhargavb left a comment

Choose a reason for hiding this comment

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

I just tried this out. The didChangeConfiguration requests are coming through to the server now yay! However, I noticed that the response received by the server for workspace/configuration requests aren't correct. No matter what setting I set on the client, the result from this request is always the default value for each property. Calling vscode.workspace.getConfiguration() on the client returns the correct values so this isn't an issue with the client.

Another thing I noticed after upgrading to your latest commit is that I keep seeing a bunch of these exceptions. They weren't happening before afaik,
image

global.json Outdated
@@ -1,7 +0,0 @@
{

Choose a reason for hiding this comment

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

Why was this deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

There was a build issue, I mainly had it in there for local development (since I have the .net 5 installed) but it isn't strictly needed for CI.

Choose a reason for hiding this comment

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

I see. I asked because when I tried to build locally with a .net 5 sdk, it failed. I had to pin it to 3.1.402 to make it work.

…e). Added ability to disable default handlers for language client / language server. Added unit tests demonstrating Configuration.Binder, and Options usages
…ontainer for 'built-in' handlers, these were overriding any custom ones that were added later
…uestContext is given, then that handler is used.
@david-driscoll
Copy link
Member Author

I worked with @ajaybhargavb and I think we solved his underlying problem, and it looks like there are no more changes required. however I'll wait until I hear back if any other problems popped up.

Copy link

@ajaybhargavb ajaybhargavb left a comment

Choose a reason for hiding this comment

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

LGTM. I tested it out and looks like this fixed the issue. Thanks @david-driscoll!

@david-driscoll david-driscoll merged commit 03d7b65 into master Oct 21, 2020
@mergify mergify bot deleted the configurable-configuration branch October 21, 2020 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants