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

Footnotes #64

Merged
merged 16 commits into from
Nov 16, 2017
Merged

Footnotes #64

merged 16 commits into from
Nov 16, 2017

Conversation

kivikakk
Copy link

@kivikakk kivikakk commented Nov 8, 2017

Fixes #56. Adds support for footnotes to cmark-gfm. This is a key requirement of being able to be used where Kramdown is; for instance, Kramdown is the default Markdown renderer in GitHub Pages.

This PR does the following:

  • Generalises the map used for reference link lookups to support both reference links and footnotes.
    • It's still fairly specific to the needs of both (it's not a "generalised map" by any means), but that's fine.
  • Parses footnote definitions (block-level) and references (inline-level) according to kramdown's syntax.
  • At the parsing finalisation stage:
    • Collects definitions in one pass over the document and added to a map.
    • Collects references in another pass, assigning increasing indexes to the definitions so-referenced. (i.e. footnote #​1 is the first footnote referenced, not the first defined.)
    • Adds definitions that were referenced to the end of the AST in reference order.
  • The HTML formatter produces links and footnote sections according to Kramdown's output format (for now).
  • The CommonMark formatter roundtrips footnotes correctly.

To-do:

  • reduce duplication between reference map/footnote map
  • don't use linear search for footnote definitions when finalizing
  • fix the static unsigned int ix; hack in commonmark.c whatever it's fine
  • take our snprintf results and turn them into asserts
  • make footnote parsing optional (it costs a bit)

@kivikakk
Copy link
Author

kivikakk commented Nov 9, 2017

Would love some review from @vmg and/or @charliesome.

@kivikakk kivikakk mentioned this pull request Nov 9, 2017
src/commonmark.c Outdated
@@ -162,6 +162,8 @@ static cmark_node *get_containing_block(cmark_node *node) {
return NULL;
}

static unsigned int footnote_ix;
Copy link

Choose a reason for hiding this comment

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

Proooobably not fine. 😄

It seems like this single variable stops the whole library from being threadsafe. Can we give this some more thought? Surely the footnote index can fit in one of the many state structs we're already passing around.

Copy link
Author

Choose a reason for hiding this comment

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

TOTALLY FINE.

(Shit, yeah.)

@metasean
Copy link

metasean commented Jul 2, 2018

@kivikakk - What is the timeline for this footnote extension to be enabled in GitHub or for gists?

@kivikakk
Copy link
Author

kivikakk commented Jul 2, 2018

There are no plans to enable the extension on GitHub.com or in gists. This is for Pages only.

@metasean
Copy link

metasean commented Jul 2, 2018

@kivikakk - Needless to say, this makes me really sad, but I really appreciate your definitive answer.

talum pushed a commit that referenced this pull request Sep 14, 2021
* Add baseline test

* Some preliminary work.

* cont'd

* Add footnote reference

* Start postprocessing

* MVP: tests pass

* commonmark footnote out

* Factor out reference/footnote maps

* fix a memory leak & some asserts

* We don't assert/check snprintf elsewhere

* Remove bad linear search, extend test case

* cleanup

* man page update

* add footnotes as option

* bugfix (found in comrak first!)

* Shift static var into renderer struct
@vassudanagunta
Copy link

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.

Footnotes
4 participants