Skip to content

Added resetState function#20

Merged
tzi merged 5 commits intoElephant418:masterfrom
danhanly:fix-looping-indent-issue
Sep 7, 2016
Merged

Added resetState function#20
tzi merged 5 commits intoElephant418:masterfrom
danhanly:fix-looping-indent-issue

Conversation

@danhanly
Copy link
Copy Markdown
Contributor

@danhanly danhanly commented Sep 6, 2016

Issue reference: #19

The issue that I discovered is that I was using a single instance of the converter, injected into my constructor - but the actual parsing (call of parseString) was happening within a loop.

This meant that a large portion of the properties, such as indent, buffers etc were being carried over from one string to another.

  • String A would set the indent to > since it ends on a blockquote.
  • String B would then have > set to the indent at the beginning of its string (which is added during the handleTag_br method) regardless of whether it was using a blockquote or not.

There was no 'cleanup' of these properties between subsequent runs of parseString against the same instance. So here I added an optional variable to parseString to allow you to reset it's state. I defaulted this variable to false so that we don't break backward compatibility.

There are conceivable situations where you would want subsequent runs of parseString to inherit the properties of a previous run (a single string split into multiple parts, for instance), so I kept this in mind with this edit.

@danhanly danhanly mentioned this pull request Sep 6, 2016
@tzi
Copy link
Copy Markdown
Member

tzi commented Sep 6, 2016

Hi @danhanly!

That's a great catch! Thanks a lot 👓

It's nice of you to conceive that some situations need subsequent runs of parseString(), but it's an undocumented and unexpected feature. So I prefer to not change the API by adding a new parameter.
Note: I'll released a minor version after your fix.

It would be very nice of you to add a test case of the bug you fixed.
There is nothing I hate more than fixing the same bug twice 🔨

Thanks again for contributing to this project!

Cheers,
Thomas.

@danhanly
Copy link
Copy Markdown
Contributor Author

danhanly commented Sep 7, 2016

Would you like me to take out the flag for resetState and have it reset state as default functionality?

Happy to do this - I'll write the test case today.

@tzi
Copy link
Copy Markdown
Member

tzi commented Sep 7, 2016

Yes! It would be great 🍰


$this->assertContains('>', $bqOutput);

$lbOutput = $converter->parseString($linebreaks);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 It's OK!
This converter produce a broken markdown on master:

Test  
>   
> Linebreaks

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the exact circumstance of the issue #19 - glad it's resolved here 👍

@danhanly
Copy link
Copy Markdown
Contributor Author

danhanly commented Sep 7, 2016

@tzi amended - travis approves 😄

@tzi tzi merged commit d248586 into Elephant418:master Sep 7, 2016
@danhanly danhanly deleted the fix-looping-indent-issue branch September 7, 2016 10:59
@tzi
Copy link
Copy Markdown
Member

tzi commented Sep 7, 2016

It's available in the release v2.2.0.

Welcome to the contributors of Markdownify!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants