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 PHP-Markdown style footnotes support #271

Merged
merged 2 commits into from Jul 21, 2013

Conversation

Projects
None yet
5 participants
@brief
Contributor

brief commented Jun 16, 2013

@djui opened an identical pull request a month ago, but it has stalled. Thought I would submit my own with the requested changes.

This includes the great original work by @bdolman and @adamflorin, a fix to footnote parsing by @microjo, a test case in test/html_render_test.rb, and edits to the changelog and readme.

@robin850

This comment has been minimized.

Collaborator

robin850 commented Jun 16, 2013

Very clean and nice pull request! Thanks. @mattr- : Should we merge this one over #241?

However, if we merge this pull request I think we should add the others contributors to the changelog entry.

@mattr-

This comment has been minimized.

Collaborator

mattr- commented Jun 17, 2013

I have a few questions:

  • Why did you choose to use a linked list for storing footnotes?
  • Did you do any testing to verify that you don't have infinite recursion and/or memory leaks in your list implementation?
  • Could you add tests for when footnotes are enabled but missing in the document?

Thanks! ❤️

@brief

This comment has been minimized.

Contributor

brief commented Jun 17, 2013

I'll defer to @bdolman for the first and second points since it is his code. As for the third, I can add tests for when footnotes are enabled but either the marker or definitions are missing. Anything else?

Merge pull request #1 from sbl/footnotes
add additional footnote tests
@olivierlacan

This comment has been minimized.

olivierlacan commented Jul 9, 2013

I assumed this was already supported by redcarpet. Good job @brief, and thanks for the pull request.

@robin850

View changes

ext/redcarpet/html.c Outdated
rndr_footnotes(struct buf *ob, const struct buf *text, void *opaque)
{
if (ob->size) bufputc(ob, '\n');
BUFPUTSL(ob, "<div class=\"footnotes\">\n<hr>\n<ol>\n");

This comment has been minimized.

@robin850

robin850 Jul 11, 2013

Collaborator

It's a detail but I would update this line like this:

if (ob->size) bufputc(ob, '\n');

BUFPUTSL(ob, "<div class=\"footnotes\">\n");
BUFPUTSL(ob, USE_XHTML(options) ? "<hr/>\n" : "<hr>\n");
BUFPUTSL(ob, "<ol>\n");

This comment has been minimized.

@robin850

robin850 Jul 11, 2013

Collaborator

With that at the very top of the function (sorry forgot about that):

struct html_renderopt *options = opaque;
@robin850

This comment has been minimized.

Collaborator

robin850 commented Jul 11, 2013

Hello there,

I've just added a quick comment in the code.

I think we won't get any answer from @bdolman since his last activity was 2 months ago.

@mattr- : What do you think about merging this and see after if we can ensure there is no infinite loop?

@mattr-

This comment has been minimized.

Collaborator

mattr- commented Jul 11, 2013

If it's almost good enough, I'd merge it into a separate branch, make any
changes we need to, and then push it back to master.

On Thu, Jul 11, 2013 at 6:43 AM, Robin Dupret notifications@github.comwrote:

Hello there,

I've just added a quick comment in the code.

I think we won't get any answer from @bdolman https://github.com/bdolmansince his last activity was 2 months ago.

@mattr- https://github.com/mattr- : What do you think about merging
this and see after if we can ensure there is no infinite loop?


Reply to this email directly or view it on GitHubhttps://github.com//pull/271#issuecomment-20805542
.

@robin850

This comment has been minimized.

Collaborator

robin850 commented Jul 13, 2013

@brief : I've noticed that you have updated this PR ; thanks! 😃

@mattr- : I think we are good to merge it into a footnotes branch or whatever. Thank you!

@brief

This comment has been minimized.

Contributor

brief commented Jul 20, 2013

Ready to merge?

@mattr-

This comment has been minimized.

Collaborator

mattr- commented Jul 21, 2013

@robin850 would you mind merging this? I'm recovering from a conference and most likely won't have time to spend on this for another few days. Thanks!

@bdolman

This comment has been minimized.

bdolman commented Jul 21, 2013

Apologies for the delay, here are some answers to earlier questions:

Why did you choose to use a linked list for storing footnotes?

It matches the data structures being used for storing links, which are similar to footnotes. To be completely accurate, links are stored using a very basic hash table predefined to contain at most 8 buckets (I believe it's 8—it's been months since I've looked at the code, but that's my recollection). Footnotes are stored in a single linked list because I didn't feel like I needed something quite as optimized as links and since it was a hardcoded 8 buckets anyway, I just didn't think that 99.9% of the use cases of footnotes would benefit from the additional complexity I'd have to put in the footnotes code.

Did you do any testing to verify that you don't have infinite recursion and/or memory leaks in your list implementation?

I did not, but I have run the Clang static analyzer and fixed any issues I found there. It's also been out in the wild for many, many months on apps with user bases in the millions and we haven't seen any issues.

@mattr-

This comment has been minimized.

Collaborator

mattr- commented Jul 21, 2013

awesome! thanks for getting back to us! It's very much appreciated! 😃

@robin850

This comment has been minimized.

Collaborator

robin850 commented Jul 21, 2013

@bdolman : Awesome! Thanks a lot!

robin850 added a commit that referenced this pull request Jul 21, 2013

Merge pull request #271 from brief/footnotes locally
Add PHP-Markdown style footnotes support

Conflicts:
	test/html_render_test.rb

@robin850 robin850 merged commit 40923d2 into vmg:master Jul 21, 2013

@robin850

This comment has been minimized.

Collaborator

robin850 commented Jul 21, 2013

Finally, merge this. Thanks everyone! 🤘 ❤️

This was referenced Jul 21, 2013

@brief brief deleted the brief:footnotes branch Jul 21, 2013

@zealot128 zealot128 referenced this pull request Sep 27, 2013

Closed

Redcarpet 3.0.0 #1299

@myitcv myitcv referenced this pull request Feb 24, 2014

Merged

Footnote support #62

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment