-
Notifications
You must be signed in to change notification settings - Fork 291
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
Add more information to Link nodes (for round-trip rendering) #98
Comments
Hi @robinst, I've been looking this over since last night and I'll have some time on various Saturdays in the next few months that might serve to implement this. In the mean time: talk is cheap! :) I've reviewed #20, #12 and the appropriate spec. Regarding your notes in #20
plus the need for a generic link interface, I was thinking this:
^edit: nope. Don't like that idea after I've thought about it. or a complete duplicate/copy of Link's functionality. An inheritance decision here might also influence Then, we'll need something that references the
Emphasis mine, because I've thought of this as a
It's not a link itself, just a pointer. Thanks for reading! |
Hey! Thanks for picking this up :). So, a couple of thoughts:
Does that make sense? |
I'm happy to help, bud! Thanks for doing lots of the heavy lifting on this project.
So, to distill: do you think that the link label of a link reference definition equates to the link text of an inline link? Inline Link Example 459 - link is link text yields <p><a href="/uri" title="title">link</a></p> Ref Def Link Example 159 - foo is link label [foo] yields <p><a href="/url" title="title">foo</a></p> Proposal: link = foo
So now, having written this, I've stumbled across http://spec.commonmark.org/0.28/#reference-link, a sub part of the Link section, which could further inform on how I should visualize this. I hadn't realized there are full reference links, collapsed reference links, and shortcut reference links (which appear to be what I was modelling this stuff based upon). I've got some more reading to do! |
Yeah, I see what you're saying. I think the Anyway, I think as soon as someone starts implementing this, it should become clear and we can discuss details on the PR. |
The spec was updated to clarify that they can also be part of setext headings. The way we make these work in our parser is to be able to "replace" the active paragraph block. In that case, we still need to collect link reference definitions from it. In order to implement this, I had to separate it from InlineParser. The new way is cleaner, and allows us to add a Node to the document as well (for #98). I think there's still room for improvement: We could parse the definition as we go in ParagraphParser and collect them earlier. That might eliminate the current "double parsing" that we do in some cases.
This is part of #98 and was enabled by the refactoring of definition parsing for spec 0.29.
The first part of this is now done, see cb43ae9. |
With the The goal of this issue is now nice and clear: Preserve enough details about links in nodes to be able to render them back in their original format. |
For #12, we need to preserve link reference definitions. We also need to be able to distinguish between the different link types (inline links, autolinks, reference links).
There is an old PR which might serve as a starting point: #20 (comment)
I'm not sure yet what the final
Node
types should look like. Ideally we'd have an interface that all the links can implement for code that doesn't care about what the source representation was.If someone is interested in working on this, please comment here first so we can figure out what the end result should look like.
The text was updated successfully, but these errors were encountered: