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

Change the type of Diagnostic.message to MarkedString #250

Closed
smarter opened this issue Jun 2, 2017 · 11 comments · Fixed by #1905
Closed

Change the type of Diagnostic.message to MarkedString #250

smarter opened this issue Jun 2, 2017 · 11 comments · Fixed by #1905
Labels
diagnostics feature-request Request for new features or functionality
Milestone

Comments

@smarter
Copy link
Contributor

smarter commented Jun 2, 2017

Currently, diagnostic messages do not carry any formatting information, this forces clients to be conservative when displaying them, see microsoft/vscode#22239 and the proposed solution:

Going forward. Deprecate the current way diagnostics are rendered and make them work just like the regular hover: a Markdown string which defaults to a non-monospace font but still lets language feature providers open up monospace code blocks with triple back quotes.

@dbaeumer
Copy link
Member

We could the same that we did for CompletionItems and make this a client capability.

@razzeee
Copy link
Contributor

razzeee commented Jun 14, 2019

This is still something that would vastly improve the experience.

@dbaeumer dbaeumer modified the milestones: On Deck, Backlog Oct 31, 2019
@michaelmesser
Copy link
Contributor

michaelmesser commented Sep 28, 2021

Why was this added to on deck then moved to the backlog? Was there some issue? This combined with #1056 would be useful to the Idris 2 LSP.

@dbaeumer
Copy link
Member

Wrongly assigned it to On Deck. As you can see changes happened the same day.

@michaelmesser
Copy link
Contributor

Is this blocked because the VSCode API doesn't support markdown for diagnostic messages? Unfortunately, the last comment was in February.

@KamasamaK
Copy link
Contributor

LSP is not solely based on VS Code. If you can get this implemented in another LSP client, that should be sufficient.

@dbaeumer
Copy link
Member

There are already feature in LSP that aren't supported in VS Code. All that LSP is requiring that new feature (whether or not implemented in VS Code) need to be guarded by a capability.

@michaelmesser
Copy link
Contributor

If I made this into a PR, would it be merged?

export interface PublishDiagnosticsClientCapabilities {
    ...
    /**
     * Client supports the follow content formats for the message
     * property. The order describes the preferred format of the client.
     */
    messageFormat?: MarkupKind[]
    ...
}

export interface Diagnostic {
    ...
    message: string | MarkupContent
    ...
}

@dbaeumer
Copy link
Member

dbaeumer commented Oct 5, 2021

@michaelmesser yes, that goes into the right direction. But we need an implementation as well (can be in a non VS Code client). Otherwise we have specification that no one implements.

@michaelmesser
Copy link
Contributor

Do clients normally merge PRs for features not in the spec? Is an unmerged PR to a client sufficient? Is just implementing it in an editor plugin that extends an existing client sufficient? Is a server implementation also needed?

@dbaeumer
Copy link
Member

In VS Code Is usually do the following:

  • do a proposed implementation which actually ships in a next version of the LSP VS Code libs
  • do a proposed specification.
  • wait a little to wait for feedback
  • make both the implementation and specification final

Regarding the specific question: it is good enough if it ships in a beta version. However, an unmerged PR is too little. It might happen that the feature never makes it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
diagnostics feature-request Request for new features or functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants