-
Notifications
You must be signed in to change notification settings - Fork 197
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
Add support for parsing Markdown inside HTML #38
base: master
Are you sure you want to change the base?
Conversation
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.
Hi @adam-fowler, thanks for putting this together! I'm hoping @JohnSundell has time to look at this soon, but I found a couple of things that need to be fixed for this to be merged.
First, could you rebase this on master, so that it will merge nicely with other pending PRs?
Second, there is a string index out-of-bounds error I found when running this against the CommonMark test suite. I added a guard
statement in the review comments. I also added a couple nitpicky formatting details based on previous comments by John.
With those two changes, this PR causes CommonMark tests 122, 125, and 157 to pass, with no newly failing tests. Being able to wrap elements in styling <div>
tags will be great!
Sources/Ink/Internal/HTML.swift
Outdated
possibleMarkdown = true | ||
} | ||
} | ||
|
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.
guard !reader.didReachEnd else { break } |
The following input (example 125 in the current CommonMark spec) causes an out-of-bounds error:
<div>
*foo*
*bar*
This guard statement fixes the issue, and all other current spec tests run without crashing.
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 had to put the guard
statement slightly further up to get it to work. ie above the double newline check
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.
Oops, that's where it was supposed to be.
Tests/InkTests/HTMLTests.swift
Outdated
@@ -93,7 +93,7 @@ final class HTMLTests: XCTestCase { | |||
|
|||
XCTAssertEqual(html, "<p>Hello</p><br/><p>World</p>") | |||
} | |||
|
|||
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.
Picky, but I think John prefers not to have this indentation.
Tests/InkTests/HTMLTests.swift
Outdated
|
||
XCTAssertEqual(html, src) | ||
} | ||
|
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.
func testUnclosedHTMLWithDoubleNewline() { | |
let html = MarkdownParser().html(from: """ | |
<div> | |
*foo* | |
*bar* | |
""") | |
XCTAssertEqual(html, "<div>\n*foo*<p><em>bar</em></p>") | |
} |
Here's an extra test that catches the out-of-bounds error.
("testMarkdownBeforeHTML", testMarkdownBeforeHTML), | ||
("testMarkdownAfterHTML", testMarkdownAfterHTML), | ||
("testMultipleMarkdownInsideHTML", testMultipleMarkdownInsideHTML), | ||
("testHTMLWithDoubleNewline", testHTMLWithDoubleNewline), |
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.
("testHTMLWithDoubleNewline", testHTMLWithDoubleNewline), | |
("testHTMLWithDoubleNewline", testHTMLWithDoubleNewline), | |
("testUnclosedHTMLWithDoubleNewline", testUnclosedHTMLWithDoubleNewline), |
Add extra text case.
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.
You don't seem to have included the test. I'll add it
07e6a7e
to
d699da9
Compare
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.
Looks good to me!
I think this would help too for cases where you want to have some code hidden:
|
See CommonMark example 108 https://spec.commonmark.org/0.18/#example-108. Added GroupFragment protocol that can hold multiple fragments as HTML fragment will now need to hold its HTML plus any Markdown fragments inside the HTML block.
This fixes https://spec.commonmark.org/0.29/#example-125. Previously it was crashing
Co-Authored-By: John Mueller <[email protected]>
0c1b509
to
518d58a
Compare
See CommonMark example 108 https://spec.commonmark.org/0.18/#example-108.
Added GroupFragment protocol that can hold multiple fragments. HTML fragment comforms to this as it needs to hold its HTML plus any Markdown fragments included inside the HTML block.
HTML parsing will set state
possibleMarkdown
whenever it comes across two newlines in a row. At this point it checks for markdown characters and tries to parse them.Checkout the tests I added for examples of markdown inside HTML.
I specifically added this so I could create a div with a class attribute containing images as follows. Thus allowing me to use css to style the images.