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 definition lists that contain other lists #264

Merged
merged 1 commit into from
May 10, 2016

Conversation

moorereason
Copy link
Contributor

Fixes #263

Ping @miekg @tianon

@miekg
Copy link

miekg commented May 6, 2016

yes, this passes my tests

@miekg
Copy link

miekg commented May 6, 2016

nice short fix btw :)

@dmitshur
Copy link
Collaborator

dmitshur commented May 7, 2016

I don't completely understand how it works, and I'm not familiar with definition lists (I never use them myself). The change looks reasonable and passes all existing tests, and fixes this one, so LGTM. /cc @rtfb

@tianon
Copy link

tianon commented May 8, 2016

Tested it with my example from #235 (comment), and it works great. 👍

@@ -1175,8 +1175,9 @@ gatherlines:

if containsBlankLine {
// end the list if the type changed after a blank line
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this comment still up to date despite the code change below? Just checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should probably be updated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should I wait for your change or should I merge right away?

@moorereason
Copy link
Contributor Author

@rtfb, you can probably merge this now, and we can improve upon it later.

My main concern is that this PR only solves half of the problem. If you have a Def List that contains differing adjacent lists, the adjacent lists are merged into a single list (which is the problem that I was originally trying to fix). I'm having a hard time understanding how to detect the nesting state to fix that problem and haven't had time to devote to fixing it.

Example test case:

"Term 1\n:   Definition a\n\n    Text 1\n\n    1. First\n    2. Second\n    * Foo\n    * Bar",

@rtfb rtfb merged commit 2004188 into russross:master May 10, 2016
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.

5 participants