Conversation
|
|
||
| private async Task<Dictionary<Uri, ImmutableArray<LSP.Diagnostic>>> GetDiagnosticsAsync(Document document, CancellationToken cancellationToken) | ||
| { | ||
| var diagnostics = _diagnosticService.GetDiagnostics(document.Project.Solution.Workspace, document.Project.Id, document.Id, id: null, includeSuppressedDiagnostics: false, forPullDiagnostics: false, cancellationToken) |
There was a problem hiding this comment.
note that this says forPushDiagnostics: true. meaning that we will push in our current mode where we have not switched to 'pull diagnostics'. However, if we flip the new option to "use pull diagnostics", this will now cause us to get no diagnostics back here, so we'll push nothing.
There was a problem hiding this comment.
I want to make sure my understanding of the PR is correct
In summary:
- LiveshareInProcLanguageClient supports push only which is used in codespaces.
- PullDiagnosticsLanguageClient supports pull only when the feature flag is enabled.
So in codespace when feature flag is off:
- LiveshareInProcLanguageClient pushes all diagnostics
- PullDiagnosticsLanguageClient is not pulled from as the capability for pull = false
And when feature flag is on in a codespace (this is an unlikely scenario as setting roslyn options in a codespaces is not yet working, see #48567 . Therefore I don't much care about what happens here):
- The LiveshareLanguageServerClient will not push anything because it is a push client of the diagnostic service.
- The platform ignore pushes anyway, and only pull diagnostics from PullDiagnosticsLanguageClient (which returns results as the feature flag is on and it is a pull client for the diagnostics service)
And in a non-codespace
- LSP push is never used (has DisableUserExperience(true))
- The feature flag switches between local roslyn and LSP pull for diagnostics
Yes.
yes
yes
yes
I don't know. Depends on if this option is seen by liveshare.
that is how it should behave. I'm a little wary about this as this is all very complex. However, if they do push, it's a problem on their end.
Yes.
Yes. |
|
Hello @CyrusNajmabadi! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
No description provided.