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

Include tilde (~) in markdown syntax tokens #146417

Merged
merged 3 commits into from
Dec 13, 2022

Conversation

babakks
Copy link
Contributor

@babakks babakks commented Mar 31, 2022

This PR fixes #146219

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@geekley
Copy link

geekley commented Apr 1, 2022

@babakks Shouldn't : and ; be backslash-escaped as well? To prevent links and HTML character entities from being treated specially? E.g.:

https\://example.com
&amp\;

https://example.com
&amp;

@geekley
Copy link

geekley commented Apr 1, 2022

Also, IMO \n would be better escaped by "  \n" (2 spaces then newline) rather than "\n\n" (2 newlines).
The former makes a newline, the latter starts a new paragraph (this distinction doesn't seem to exist in github-flavored markdown, since it treats every newline as a newline; but it does in classical/vscode markdown).

Though I admit this is minor and also debatable.

EDIT: Huh? I noticed there should be now a parameter for that, but I don't see it in 1.65.0. Maybe it didn't make it into the API yet? The commit is from Nov/2020 though...

@babakks
Copy link
Contributor Author

babakks commented Apr 2, 2022

@geekley I see your point, but I think we'd better off sticking with the standard markdown definition on escapable characters (here) which does not include ; or : (though, for example, Github supports escaping them).

@geekley
Copy link

geekley commented Apr 2, 2022

@babakks
OK, but that definition doesn't include ~ either. In fact ~ is not special in original markdown, so VSCode is using some other "flavor". And I did test all three backslash-escaping chars in VSCode and they do work.

If you want to ensure reliable behavior, you'd have to know which markdown spec VSCode actually uses. I assume it's likely it may be using CommonMark, which is a much more "proper" standard than the original -- and, at least on latest version I saw now, it does include escaping for those characters and many others as you can see here:
https://spec.commonmark.org/0.30/#backslash-escapes

If you still don't want to rely on backslash escaping, there's also the option of preceding them with a comment (I tested that too on all three): <!---->: and <!---->;
In the ~ case, it also did seem to work, but if you want to be extra safe it might be prudent to surround it on both sides (since it can be used on start or end of strikethrough): <!---->~<!---->

But I think the best solution would be confirming whether it's using CommonMark and which version, and then include all escaping of that spec's version, specially because we may be missing some other edge cases.


EDIT: In fact, even in the oldest version listed, the set of escapable chars in CommonMark was the same, it seems:
https://spec.commonmark.org/0.5/#backslash-escapes
So, if VSCode's markdown comes from, for example, a npm dependency on the popular markdown-it (I didn't check if that's the case), then that set of chars is surely the right one to use.

@geekley
Copy link

geekley commented Apr 2, 2022

Ah OK, VSCode API does state that it's using CommonMark:
@types/vscode v1.65.0 index.d.ts

    /**
     * Human-readable text that supports formatting via the [markdown syntax](https://commonmark.org).
     *
     * Rendering of {@link ThemeIcon theme icons} via the `$(<name>)`-syntax is supported
     * when the {@linkcode supportThemeIcons} is set to `true`.
     *
     * Rendering of embedded html is supported when {@linkcode supportHtml} is set to `true`.
     */
    export class MarkdownString {

So, the full list of chars should be:

!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~

!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~

@babakks
Copy link
Contributor Author

babakks commented Apr 4, 2022

@geekley I think you're right. it makes sense to update the character set.

@jrieken Do you agree?

@jrieken jrieken added this to the January 2023 milestone Dec 13, 2022
@jrieken
Copy link
Member

jrieken commented Dec 13, 2022

I generally agree but let's keep this PR focused on the ~ issue

@jrieken jrieken merged commit 45d2254 into microsoft:main Dec 13, 2022
@babakks babakks deleted the escape-tildes-in-md branch December 13, 2022 08:59
@geekley
Copy link

geekley commented Dec 14, 2022

I generally agree but let's keep this PR focused on the ~ issue

@jrieken @babakks Is there an open issue/PR for the rest of the CommonMark chars that should be escaped, so that I can subscribe to it?

@github-actions github-actions bot locked and limited conversation to collaborators Jan 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vscode API bug: MarkdownString appendText does not escape tilde
4 participants