-
Notifications
You must be signed in to change notification settings - Fork 105
Custom handlers not override built in handlers correctly #392
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
Conversation
david-driscoll
commented
Oct 13, 2020
•
edited
Loading
edited
- Fixed a bug in configuration (configuration values were case-sensitive).
- Added unit tests demonstrating Configuration.Binder, and Options usages
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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,
global.json
Outdated
@@ -1,7 +0,0 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this deleted?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
010d6c8
to
ca564c0
Compare
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. |
There was a problem hiding this 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!