-
Notifications
You must be signed in to change notification settings - Fork 158
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
Unicode symbols in Markdown #90
Comments
@nhirschey , it is difficult to tell 100% if that fixed it because it's not clear what the original problem in this ticket was. However, the fix in #464 would, I believe, have had an impact on the bug in this ticket and the fact that I can't reproduce it in as in the examples I've provided would appear to be the case. My guess is that we could close this ticket and re-open it unless you can reproduce the original bug. |
Thanks for checking @DavidSSL. Before closing we should add a few more tests to make sure. For future reference to me and others, relevant commonmark test cases are around the below case in https://spec.commonmark.org/0.30/spec.json {
"markdown": " & © Æ Ď\n¾ ℋ ⅆ\n∲ ≧̸\n",
"html": "<p> & © Æ Ď\n¾ ℋ ⅆ\n∲ ≧̸</p>\n",
"example": 25,
"start_line": 650,
"end_line": 658,
"section": "Entity and numeric character references"
} |
You're welcome @nhirschey. I was inspired by your Amplifying FSharp talk to contribute back even though I don't use FSharp.Formating :). Out of curiosity, since I am not so familiar with this domain, for the common mark tests above, the result you'd be expecting is this? |
@DavidSSL that's very kind of you to say! It'd be great to have your help. I believe your example is rendering correctly. One thing you could try .... there are tests for the commonmark spec in this library but the library does not pass all of them so some of them are disabled. It looks like the "Entity and numeric character references" tests are disabled currently. If you want, perhaps try enabling them by adding them here?
If |
@nhirschey, you will have to give me some time to look at this because I'm not so familiar with the domain and the code. Thanks for pointing me in the right direction though. I should be able to figure things out. Having said that, the following: FSharp.Formatting/tests/commonmark_spec.json Lines 2307 to 2312 in 09491d1
does not look correct at all. I would assume that the HTML should be like what I have in the post above. Correct? Moreover, it would appear that #464 is actually incorrectly implemented. FSharp.Formatting/tests/FSharp.Markdown.Tests/Markdown.fs Lines 21 to 25 in 09491d1
because when I run this via dingus, I get: As you can see, I do need further guidance in terms of expected behaviour. |
@nhirschey I think that I understand the problem space better. However, that is a bigger piece of work than envisaged. Basically, if you use the https://spec.commonmark.org/0.30/spec.json, tests belonging to the I will certainly give it a go but it could be quite a slog. |
Thanks for digging into this @DavidSSL. For sure it's too much to get this library fully complying with the commonmark spec in one go; that's a huge task. But it would be awesome if you happen to find a bite-size chunk that you can fix to help push us towards that goal. No pressure, no rush. Even simply through your investigation here I've learned some things, thank you. Regarding, [<Test>]
let ``Don't double encode HTML entities outside of code`` () =
"a > & © b"
|> Markdown.ToHtml
|> should contain "<p>a > & © b</p>" I agree that it Some existing tests account for "improper" markdown parsing, and they will break when the parsing is better. When I wrote the below test for emphasis based off the commonmark spec, I knew the actual correct value should be FSharp.Formatting/tests/FSharp.Markdown.Tests/Markdown.fs Lines 892 to 895 in 09491d1
|
@nhirschey things are clear now. I'll create tickets and link back to this issue to try and move the needle towards compliance. I might not succeed but I'll sure try and give it a go. |
Unicode html tags like α
α
don't seem to work in .fsx or Markdown comments in code. Work-around with latex syntax is pretty simple though.The text was updated successfully, but these errors were encountered: