search

gomarkdown / markdown

markdown parser and HTML renderer for Go
Other
1.36k stars 171 forks source link

Handle bold/strong nested inside italics/em #307

Closed cddude229 closed 1 month ago

cddude229 commented 3 months ago

While trying out this library, my initial input ran up against some edge cases. (It handled everything else perfectly though, which is awesome.). I decided to take a stab at fixing the bugs, which is this PR. I've broken down every test case addition + related fix into its own commit for easy review.

Here's the summary of changes in input/output: Input Output in production Output after this PR
*italics **and bold** end* <p>*italics <strong>and bold</strong> end*</p> <p><em>italics <strong>and bold</strong> end</em></p>
*italics **and bold*** <p>*italics <strong>and bold</strong>*</p> <p><em>italics <strong>and bold</strong></em></p>
***bold** and italics* <p>*<strong>bold</strong> and italics*</p> <p><em><strong>bold</strong> and italics</em></p>
*start **bold** and italics* <p>*start <strong>bold</strong> and italics*</p> <p><em>start <strong>bold</strong> and italics</em></p>
*improper **nesting* is** bad <p>*improper <strong>nesting* is</strong> bad</p> <p><em>improper **nesting</em> is** bad</p>
miekg commented 3 months ago

I don't see how the testcase makes the case for this PR?

Don't understand the random i +=2. Comments also didn't change even though the code around it changed very much

kjk commented 3 months ago

@miekg It does seem to fix the following: In current code: *italics **and bold** end* => <p>*italics <strong>and bold</strong> end*</p> See https://tools.arslexis.io/goplayground/#iPKnCL5art6

Per babelmark, most other return: <p><em>italics <strong>and bold </strong> end</em></p> see: https://babelmark.github.io/?text=*italics+**and+bold**+end*

The case *improper **nesting* is** bad gets closer to what majority does but not fully (see https://babelmark.github.io/?text=*improper+**nesting*+is**+bad)

cddude229 commented 3 months ago

I don't see how the testcase makes the case for this PR?

Don't understand the random i +=2. Comments also didn't change even though the code around it changed very much

Apologies - I got lazy at the end of the work day and should have provided more context. I've updated the PR description and split the main commit into two (to better highlight what each change does) in hopes it better communicates the goals.

I also realized another edge case and added a test for that + fixed it. This reverted the optimization case I was talking about as the bug was there instead.

Lemme know if you need more information!

cddude229 commented 2 months ago

@miekg Just following up on this, as I think I addressed all your concerns. Let me know your thoughts and if you need me to do anything else!

kjk commented 1 month ago

Thanks, I pushed it in https://github.com/gomarkdown/markdown/commit/77f4768f716dcbd724917f1bade5f8c07e8a8cc1

Had to resolve a conflict and add a fix for infinite loop from #311 so couldn't just merge it