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 lists with paragraph content #8

Merged
merged 1 commit into from
Dec 15, 2014

Conversation

mojavelinux
Copy link
Collaborator

Another super hack, though hopefully the PR gives you the gist of the problem. List items that are separated by a blank line end up having a list marker on a separate line from the content...which breaks the list in AsciiDoc. I've done a quick hack here to not put a blank line before start of a paragraph when inside a list.

In general, I think the serializer should not put a blank link at the top of content, only at the bottom of content. That way, you can control better where endlines get placed. When they are added above and below, you lose a lot of control when using the recursive decent visitor strategy.

@mojavelinux
Copy link
Collaborator Author

To clarify where I stand on this PR, focus on the test, not the implementation. I encourage you to reimplement it in a more sensible way.

@bodiam
Copy link
Collaborator

bodiam commented Dec 12, 2014

Hi Dan,

Thanks for your pull request! I'll investigate tonight. Regarding your comment about adding whitespace below the content, I completely agree, but I admit I made some hacks to get as many tests as possible working. I can see if I can remove the whitespace addition at the beginning of paragraphs!

Thanks again, it's amazing how many pull requests you can do in a day!

Erik

@mojavelinux
Copy link
Collaborator Author

Actually, I thought of a better way to implement this one. I'll update my pull request as soon as I get a chance.

@mojavelinux
Copy link
Collaborator Author

I can see if I can remove the whitespace addition at the beginning of paragraphs.

I think I have an idea about how to do this cleaner. I know this is the most tedious aspect of the converter, hands down. I'm going to do some experimentation to see if my idea has any legs.

@bodiam
Copy link
Collaborator

bodiam commented Dec 13, 2014

Super, I'm curious about it! It's 1am here now, but I will have a check in the morning to see what kinds of things you changed in both the projects. Again, thanks for the feedback and the pull requests, my mailbox is working overtime! ;-)

@mojavelinux
Copy link
Collaborator Author

...in short, the summary of what I'm going to try to do is manage endlines at the top of blocks and also use introspection of parent & sibling nodes to decide when it is appropriate to use endlines.

Note that to do complex content on a list item, we need to use the list continuation lines. It's tough to manage, but I've done similar logic inside of the Asciidoctor parser itself, some I'm very familiar with the requirements and what to be aware of.

We really want to avoid a blanket search and replace in a postprocessing step because AsciiDoc is highly contextual and you end up taking away spaces where they are meaningful (or, in the most obvious case, inside code listings where it should be verbatim).

@mojavelinux
Copy link
Collaborator Author

Fortunately, the test suite is going to be a huge asset to us in getting this right, because we can easily verify all the edge cases with whatever solution we implement.

- clean superfluous nodes from AST
- don't add endline before paragraph if paragraph is primary list item text
- quote alt text if it contains a comma
- quote link text if it contains a comma
- handle linked images in a cleaner way
@mojavelinux
Copy link
Collaborator Author

This one should be good to go now. I had to do some AST cleanup to be able to inspect the parent nodes, as well as implement a parent locator method. Pegdown doesn't have the nicest AST to work with, but what we have now at least at lot more context aware.

Btw, I have no idea what the story is with the RootNode objects in the middle of the AST, which is why I removed them. In many cases the SuperNode objects get in the way, so I remove them if they contain only one child.

@bodiam bodiam merged commit 3d2f068 into markdown-asciidoc:master Dec 15, 2014
@bodiam
Copy link
Collaborator

bodiam commented Dec 15, 2014

Tricky merge, but it's done, all tests pass! Thanks!!

@mojavelinux mojavelinux deleted the list-of-paragraphs branch December 16, 2014 06:44
@mojavelinux
Copy link
Collaborator Author

Nice!

As I mentioned, I'm going to file an issue upstream to make the parent node available in the Pegdown AST because it's really painful to not have it available. We've found a way around it, but we shouldn't need this hack.

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.

2 participants