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

Get rid of Link_def node #217

Merged
merged 1 commit into from
Jul 5, 2020
Merged

Get rid of Link_def node #217

merged 1 commit into from
Jul 5, 2020

Conversation

nojb
Copy link
Contributor

@nojb nojb commented Jul 5, 2020

As observed by @shonfeder, the Link_def node is no longer useful in the AST. Let's remove it.

Copy link
Collaborator

@shonfeder shonfeder left a comment

Choose a reason for hiding this comment

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

LGTM!

@nojb nojb merged commit 8be53f2 into master Jul 5, 2020
@nojb nojb deleted the remove_link_def branch July 5, 2020 07:02
@jfrolich
Copy link
Contributor

jfrolich commented Jul 6, 2020

Why is it no longer useful? Isn't it good to have the link definitions in the AST to disambiguate between the two uses of links? Especially if we have an AST -> markdown printer (you probably want to preserve the defs in that case).

@nojb
Copy link
Contributor Author

nojb commented Jul 6, 2020

Why is it no longer useful? Isn't it good to have the link definitions in the AST to disambiguate between the two uses of links?

It is no longer useful because reference links are resolved at parsing time since #209. Since we do not keep a reference to the link definition in the AST, having the link definition node does not serve any purpose.

There is the larger question of how lossy/lossless do we want the AST to be. Lately I have been leaning towards making the AST easier to use (and so more lossy). Preserving link reference and definition nodes allows to reconstruct the original markdown, but makes the AST harder to use, as the user needs to resolve link references.

Especially if we have an AST -> markdown printer (you probably want to preserve the defs in that case).

Indeed, if we add such a printer, it won't be able to recreate link references, and it will just inline the link. I am not too worried about this (but opinions welcome). The only use I can think of for the AST->markdown printer (apart from pretty printing) is for generating markdown programmatically; the use of link references/definitions does not seem to be crucial for this.

@nojb
Copy link
Contributor Author

nojb commented Jul 6, 2020

As you observed, another case of lossy-ness in the AST is that we do not record which delimiter was used for emphasis/strong emphasis (_ or *) or the number of backquotes used to delimit code blocks (this used to be the case, but added noise to the AST).

@nojb
Copy link
Contributor Author

nojb commented Jul 6, 2020

#223

@jfrolich
Copy link
Contributor

jfrolich commented Jul 6, 2020

I understand you want to reduce complexity, but the case can be made that link definitions are semantically different (so can be treated differently in a particular printer). About the star or underscore emphasis a case can be made that there is no difference, but it can be pretty handy for a printer to allow supporting a richer feature set while not extending the syntax. These add a bit of complexity to the AST, but it can also easily be ignored by printers?

An addition that I don't really understand is the attributes that are added to the AST.

@nojb
Copy link
Contributor Author

nojb commented Jul 6, 2020

An addition that I don't really understand is the attributes that are added to the AST.

This is an extension of the standard used by Pandoc which is quite handy to enrich the AST with custom metadata. In the HTML printer, they correspond to usual attributes. See https://github.com/jgm/commonmark-hs/blob/master/commonmark-extensions/test/attributes.md for the spec.

@jfrolich
Copy link
Contributor

jfrolich commented Jul 6, 2020

Ah ok thanks for referencing, I was wondering where that was coming from. That can definitely be handy indeed.

@nojb
Copy link
Contributor Author

nojb commented Jul 6, 2020

Thanks for the feedback re: keeping link references in the AST. I want to mull it over, but I see that it could be useful in some cases.

shonfeder pushed a commit to shonfeder/omd that referenced this pull request Jan 22, 2021
shonfeder pushed a commit that referenced this pull request Apr 11, 2021
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