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

Fix #466 #467

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fix #466 #467

wants to merge 7 commits into from

Conversation

aidantwoods
Copy link
Collaborator

Beginnings of a potential fix to #466 – provided this doesn't break anything unexpectedly

Beginnings of a potential fix to erusev#466 – provided this doesn't break anything unexpectedly
…r a new block exists on the current line at a lesser indentation: if so, interupt current block
@aidantwoods
Copy link
Collaborator Author

Seems that the Commonmark reference parser is opting to have block structures that aren't properly indented interrupt the previous block structure (which seems sensible). Rather than to embed the new block structure within the previous (which is what Parsedown is currently doing).

screen shot 2017-01-23 at 14 21 49

These changes should allow the block* methods to set an indentation level for the block, but if they don't, then the indentation is set to be the current line indent (I think this is sensible?)

Have had to change the way list indentation is calculated (so that initial whitespace and marker width is taken into account, I don't think its quite per spec yet, so the failed tests are expected until I figure out the best way to do that 😉

The added check:

if (isset($CurrentBlock['indent']) and isset($NewBlock) and $NewBlock['indent'] < $CurrentBlock['indent'])
{
    $CurrentBlock['interrupted'] = true;
}

does fix the #466

(will have to go through the test results now to double check nothing unexpected has broken)

@aidantwoods
Copy link
Collaborator Author

aidantwoods commented Jan 23, 2017

Okay, so looks like this no longer breaks anything in the Parsedown tests.

Not fit for merge yet though, still need to make sure list indents are done correctly.

What I've done so far is make sure that an initial list won't have an indentation of zero (which it would do previously) so we can differentiate between a block started within the list, and one started after but alongside the list (lesser indentation level than the content of the list, and should thus interrupt the list).

But would still like to make sure the indentation level I'm assigning is moving to something more inline with the commonmark spec.

@PhrozenByte
Copy link
Contributor

Side note @erusev: Merging #423 and #440 would make testing changes against the CommonMark specs significantly easier... 😉

Looks good to me @aidantwoods. Have you checked your changes against the CommonMark tests yet? There might be some related tests that allow us to kill two birds with one stone. 😃

@aidantwoods
Copy link
Collaborator Author

@PhrozenByte

It fixes some, and breaks some (hard to know if previously passed tests were intentional)
screen shot 2017-01-24 at 00 31 29

There are a few to do with references (some of which are apparently solved by undoing that last commit that prevents the definition log being written to on a block test o.0)

This one inserts an extra paragraph if the list content falls behind the marker
screen shot 2017-01-24 at 00 07 59

I think the most serious breakages are #85
screen shot 2017-01-24 at 00 11 54
which inserts an extra line into code

I solved that by specifying the correct indent in the blockCode method.

And the ones to do with empty list item parsing (no whitespace in item, just the delimiter)
screen shot 2017-01-24 at 00 29 41

Any ideas on what to do about these?

@aidantwoods
Copy link
Collaborator Author

Oh, and I know you're not really meant to do this in comments anymore, but +1 on merging #423 and #440! 😉

Had to go and download the working CommonMark tests from @PhrozenByte's fork, and then add them in manually to different snapshots of Parsedown to compare the test results

…ts, but fails a few in native tests. Interesting...
@aidantwoods
Copy link
Collaborator Author

Oh, and obviously there are a few nice list tests that no longer fail

screen shot 2017-01-24 at 00 48 24

screen shot 2017-01-24 at 00 48 33

screen shot 2017-01-24 at 00 50 02

@aidantwoods
Copy link
Collaborator Author

aidantwoods commented Jan 24, 2017

One of the things I'm noticing, is that block interrupts are going to have to be decoupled from being caused by a blank line, e.g. on this fall-through condition which is implied by an interrupt

$Block['li']['text'] []= '';

@erusev
Copy link
Owner

erusev commented Mar 5, 2017

I'll need some time to review this.

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.

3 participants