Repository navigation
Markup message support push - #1861
Marvin Brouwer (Marvin-Brouwer) wants to merge 11 commits into
Conversation
…age-server-protocol docs
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
@microsoft-github-policy-service agree |
| * in pull diagnostic messages. | ||
| * | ||
| * @since 3.18.0 | ||
| * @proposed |
There was a problem hiding this comment.
3.18.0 shipped. We can't mark anything as propsed for 3.18
There was a problem hiding this comment.
That is absolutely fair, I figured since https://github.com/microsoft/language-server-protocol does have a proposed marking someone might have missed it.
But if we can't do it after the fact I will revert it.
Are you sure though? Since it does really say @proposed here:
https://github.com/microsoft/language-server-protocol/blob/gh-pages/_specifications/lsp/3.19/language/pullDiagnostics.md?plain=1#L61
There was a problem hiding this comment.
I see you answered in that PR, I will remove the proposed tag
There was a problem hiding this comment.
Removed it
| }, | ||
| "documentation": "The diagnostic's message. It usually appears in the user interface.\n\n@since 3.18.0 - support for MarkupContent. This is guarded by the client\ncapability `textDocument.diagnostic.markupMessageSupport`.", | ||
| "since": "3.18.0 - support for MarkupContent. This is guarded by the client\ncapability `textDocument.diagnostic.markupMessageSupport`." | ||
| "documentation": "The diagnostic's message. It usually appears in the user interface.\n\n@since 3.18.0 - support for MarkupContent in pull diagnostics.\nThis is guarded by the client capability\n`textDocument.diagnostic.markupMessageSupport`.\n\n@since 3.19.0 - support for MarkupContent in push diagnostics.\nThis is guarded by the client capability\n`textDocument.publishDiagnostics.markupMessageSupport`.", |
There was a problem hiding this comment.
I assume you generate the file using npm run generate:metaModel. Right?
There was a problem hiding this comment.
Yes, I did
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The pull capability remains incorrectly unmarked as proposed, and the new negotiation path lacks integration-test coverage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 3
Open (3)
What changed in this PR
Adds push-diagnostic negotiation for MarkupContent messages alongside existing pull support.
Changes:
- Adds the proposed push capability and generated metadata.
- Clarifies push versus pull diagnostic documentation.
- Explicitly advertises unsupported markup messages in the client.
| File | Description |
|---|---|
types/src/main.ts |
Documents push and pull capability guards. |
protocol/src/common/protocol.ts |
Adds the push capability. |
protocol/src/common/protocol.diagnostic.ts |
Clarifies the pull capability. |
protocol/metaModel.json |
Updates generated protocol metadata. |
client/src/common/client.ts |
Advertises push support as disabled. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Fix for #1860.
3.18 added
MarkupContentdiagnostic messages, guarded bytextDocument.diagnostic.markupMessageSupport.The guarded
messagefield is onDiagnostic, which both the push and pull models use, but the capability only exists onDiagnosticClientCapabilities.So a push-only server has no way to negotiate it.
In #1860 Dirk Bäumer (@dbaeumer)'s preference was to mirror the property rather than reword the existing one:
Notes
@proposed, to matchpullDiagnostics.mdin the specification.Reverted per Dirk Bäumer (@dbaeumer)'s review: 3.18.0 has shipped, so the property can't be marked proposed
retroactively. The new push capability keeps
@proposed, since 3.19 is still under development.client.tsline is not a behaviour changeThe property is optional, so omitting it already means unsupported. It's there so that both capabilities carry the same comment and turn up together when the flag is eventually enabled; otherwise it's easy to flip the pull one and leave push behind.
contributing.mdcalls avscode-languageclientreference implementation desirable.There isn't one here because VS Code can't render markdown diagnostic messages yet (Allow diagnostics messages to have markdown (or formatted text) content vscode#54272, feature: Allow diagnostics messages to have markdown content vscode#214051), which is why
markupMessageSupportis hardcodedfalseon both paths.