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

Fixes issues #72 and #78 #91

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions protocol.md
Original file line number Diff line number Diff line change
Expand Up @@ -1069,7 +1069,7 @@ _Response_
* result: `Hover` defined as follows:
```typescript
/**
* The result of a hove request.
* The result of a hover request.
*/
interface Hover {
/**
Expand All @@ -1078,13 +1078,24 @@ interface Hover {
contents: MarkedString | MarkedString[];

/**
* An optional range
* An optional range is a range inside a text document
* that is used to visualize a hover, e.g. by changing the background color.
*/
range?: Range;
}
```
Where `MarkedString` is defined as follows:
```typescript
/**
* The marked string is rendered:
* - as markdown if it is represented as a string

Choose a reason for hiding this comment

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

Not sure this is something the server should specify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which part? It does not expose anything about the markdown format, so it is still up to the server to decide what kind of markdown to send. I've just wanted to point out that there is semantical difference depending on the type of a string. We've already had troubles to decided what to send.

Choose a reason for hiding this comment

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

Specifying default is markdown is IMO not making the protocol better.
If your patch is amending the current version of the protocol, then it should say that the marked string is usually rendered as markdown since it may not be the choice taken by all clients.
If your patch is a proposal for a future version of the protocol, then simply finding a way to avoid ambiguity (remove string and keep only MarkedString) seems much better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just state how they are handled now. Agreed about usually for markdown.

I don't see why MarkedString is better than string? I would prefer strings, since they are generic and can encode code blocks. It would simplify specification to a simple fact that marked string are usually rendered as markdown. Opposite is not true, one need to come up with a special handling of a language to allow arbitrary markdown.

Choose a reason for hiding this comment

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

Why forcing usage of markdown in the protocol when some other formats exists? Some servers may prefer HTML or asciidoc. Giving the choice to the server is actually a good thing, and if we want to give choice, then the MarkedString needs to remain to explicit the language; and if we have to keep MarkedString for that, why also keeping string?

Copy link
Contributor

Choose a reason for hiding this comment

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

giving choice to the server comes at the cost that clients need to support the different formats.

Copy link
Member

Choose a reason for hiding this comment

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

Actually the idea was to use markdown :-) As the content definition says it is a MarkedString | MarkedString[]. So I will merge the PR in to make this more clear since it is todays state. I opened #94 to continue the discussion whether this was a good idea or not.

* - as code block of the given langauge if it is represented as a pair of a language and a value
*
* The pair of a language and a value is an equivalent to markdown:
* ```${language}
* ${value}
* ```
*/
type MarkedString = string | { language: string; value: string };
```
* error: code and message set in case an exception happens during the hover request.
Expand Down