-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Add diagnostic provider for clients without dynamic registration #77984
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
6ccb054
to
b8596a3
Compare
b8596a3
to
d2ca595
Compare
@dotnet-policy-service agree |
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.
Apologies for the delay here
@@ -25,15 +25,15 @@ public DiagnosticRegistrationTests(ITestOutputHelper? testOutputHelper) : base(t | |||
} | |||
|
|||
[Theory, CombinatorialData] | |||
public async Task TestPublicDiagnosticSourcesAreRegisteredWhenSupported(bool mutatingLspWorkspace) | |||
public async Task TestPublicDiagnosticSourcesAreRegisteredWhenSupported(bool mutatingLspWorkspace, bool dynamicRegistration) |
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.
This test doesn't seem quite complete - adding the test parameter is fine (or it could be a separate test), but I would expect the test to assert that no dynamic capabilities (and no separate sources) are registered when the client does not support dynamic registration.
Instead I would expect the test to assert that there is only the single static registration.
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.
Sorry. I pushed this change before i was able to run the test by mistake.
The registrations are empty without dynamic registration, which i think makes sense.
I suppose i should look up the server capabilities somehow. Could you guide me in the right direction?
No worries with the delay. I just appreciate that you take the time.
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.
The registrations are empty without dynamic registration, which i think makes sense.
Yup I believe so.
I suppose i should look up the server capabilities somehow. Could you guide me in the right direction?
On the testLspServer
instance, you should be able to call GetServerCapabilities - https://github.com/dotnet/roslyn/blob/main/src/LanguageServer/Protocol.TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs#L791
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.
Thanks.
I have added an if / else on dynamicRegistration. Would you prefer an early return in the test instead?
f08c604
to
90cc3f7
Compare
c00cf95
to
2415eae
Compare
2415eae
to
f650824
Compare
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!
Closes #76624