-
Notifications
You must be signed in to change notification settings - Fork 818
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
Fixes issues #72 and #78 #91
Conversation
Hi @akosyakov, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
*/ | ||
range?: Range; | ||
} | ||
``` | ||
Where `MarkedString` is defined as follows: | ||
```typescript | ||
/** | ||
* The marked string is rendered: | ||
* - as markdown if it is represented as a string |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
No description provided.