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

Conversation

akosyakov
Copy link
Contributor

No description provided.

@msftclas
Copy link

msftclas commented Oct 9, 2016

Hi @akosyakov, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants