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

add diagnostic dynamic long description #1825

Open
wants to merge 6 commits into
base: gh-pages
Choose a base branch
from

Conversation

RobbyCBennett
Copy link

@RobbyCBennett
Copy link
Author

@microsoft-github-policy-service agree company="Scientific Toolworks, Inc."

@dbaeumer
Copy link
Member

@RobbyCBennett tahnks for the PR. Changing the spec is the easy part :-). To contribute additional functionality to the spec a reference implementation is needed as well. See https://github.com/microsoft/language-server-protocol/blob/main/contributing.md

I will keep the PR open for a while but without a reference implementation I will not be able to merge the PR.

@jwortmann
Copy link

Isn't this a breaking change?

Currently, the href field is mandatory. With this PR, it would become optional. So any client which unconditionally tries to read href might crash if the field doesn't exist.

@RobbyCBennett
Copy link
Author

To contribute additional functionality to the spec a reference implementation is needed as well.

Thank you. I was just looking to get some feedback before the reference implementation is made.

Isn't this a breaking change?

Yes, I will change it to accommodate for language clients that will crash after assuming the href field is defined.

Perhaps instead, it could be simplified to be part of Diagnostic, as a new field longMessage that could be just below message. I think that this would be more clear and it also wouldn't break language clients.

1348fc7

Comment on lines 41 to 51
/**
* An optional property to describe this diagnostic's message with more
* details. For example:
* The type name "int_8_t" is unknown. Did you mean any of these types?
* - int
* - int8_t
* Read more about the built-in types [here](file:///path/to/docs.md).
*
* @since 3.18.0
*/
longMessage?: string | MarkupContent;
Copy link
Contributor

@rchl rchl Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming of the property can suggest that it's an alternative version of message. Not sure if the intention would be that clients show both or choose one.

If the intention is to show both then maybe this should be called description...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling it description would be too similar to the property called codeDescription, which is a general/static description of the diagnostic.

In contrast, I thought about calling it longMessage because it is similar to message but provides more detail. Perhaps something like messageDetails would be more appropriate. I think that would infer that message is still important.

@RobbyCBennett
Copy link
Author

RobbyCBennett commented Oct 16, 2023

I didn't follow https://github.com/microsoft/language-server-protocol/blob/main/contributing.md because it seems to be intended for adding new types and methods.

Reference implementation:

Step 1: (this PR)

  • Add messageDetails field to Diagnostic

Step 2: PR in VSCode repo:

  • Add messageDetails field to Diagnostic (this is a different Diagnostic interface, defined in src/vscode-dts/index.d.ts)

Step 3

  • Update the projects that get their 2 Diagnostic types from these 2 repos (like the npm module @types/vscode), which should prepare for step 4.

Step 4 (PR in VSCode Language Server Node repo):

  • Change asDiagnostic function in createConverter and add asMessageDetails function (in client/src/common/codeConverter.ts)
  • Change asDiagnostic function in createConverter and add asMessageDetails function (in client/src/common/protocolConverter.ts)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info-needed Issue requires more information from poster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants