cmark extensions #28

kivikakk · 2016-09-13T07:49:27Z

This is not yet ready for merge, but I figure opening a PR is an okay idea at this stage.

In short:

it uses my extensions branch of cmark: https://github.com/kivikakk/cmark/tree/extensions
- this adds extensibility based on Implement parsing extensions commonmark/cmark#123, but in such a way that the extensions do not require changes to the core code
- it also doesn't do any searching and autoloading of shared objects to find extensions (!)
we register all the core extensions included in this branch
the commonmarker user can then elect which extensions to enable at parse time
added rendering back to commonmark!

Fixes #22.

gjtorikian · 2016-09-13T20:35:06Z

Not sure if you're aware but it's totally fine for the Windows build to fail: #22

I should get on that....one day. 😞

Other than that, this is looking great. The two things that stand out are:

obviously, all the XXX should raise comments
I prefer to use symbols for immutable strings, so if extensions could be an array of symbols, instead, that'd be great.

I'm not nervous about switching the submodule to your fork. I am nervous about the core CMark stuff getting improvements, that might not trickle down to your fork. For example, I routinely run update_submodule on a new CMark release and just bump the gem: #21

Could/Would you be willing to also incorporate those upstream C changes into your fork, so that I can continue to release new gems with those updates? If not, I'm totally open to any other suggestions or improvements on the workflow here. For example, could any of the C extension code be written directly to this repo, rather than maintained in a separate submodule?

kivikakk · 2016-09-14T00:54:59Z

Haha, whoops! If I ever get a Windows dev environment going I might look into it.

I could've sworn I had a good reason to use symbols instead of strings — maybe it was because one of the extension names had a hyphen in it. At any rate, I'll change it to use symbols there; will look much better!

I'm super happy to track the upstream cmark and integrate — hopefully it'll only be a matter of time before this code does get merged upstream, so it'll be helpful for me to do this anyway, to make sure it always stays merge-ready. :)

Unfortunately the nature of the changes (adapting cmark to be extensible) means we can't easily separate it yet. I'm hoping the core changes will land, and then we'll be able to keep the extensions proper in this repo if we wanted/needed.

kivikakk · 2016-09-14T02:16:57Z

Symbols are in, XXX are out, upstream is merged. :)

kivikakk · 2016-09-14T02:20:50Z

btw, Windows build failures currently look a little different to #22; might be http://help.appveyor.com/discussions/problems/4294-all-builds-say-specify-a-project-or-solution-file-the-directory-does-not-contain-a-project-or-solution-file ?

…f54484bbb03423f3dfc0b2f86080

We get conflicts in msxml.h if we don't do this. It looks like the CMARK_ prefix on our enum is getting ignored by the compiler somehow ...?!

This at least has it compiling with RubyInstaller + DevKit.

kivikakk · 2016-09-15T15:06:01Z

Windows is all fixed. Let me know if there's anything else you'd like addressed!

gjtorikian · 2016-09-16T21:36:44Z

😹

gjtorikian · 2016-09-16T21:37:56Z

appveyor.yml

+  - bundle -v
+
+test_script:
+  - bundle exec rake


OCD ALERT: can this end in a newline please? 🙏

gjtorikian · 2016-09-16T21:40:02Z

lib/commonmarker.rb

@@ -15,26 +15,28 @@ module CommonMarker
  #
  # text - A {String} of text
  # option - Either a {Symbol} or {Array of Symbol}s indicating the render options
+  # extensions - An {Array of String}s indicating the extensions to use


Should be {Array of Symbol}s.

gjtorikian · 2016-09-16T21:40:04Z

lib/commonmarker.rb

    html.force_encoding('UTF-8')
  end

  # Public: Parses a Markdown string into a `document` node.
  #
  # string - {String} to be parsed
  # option - A {Symbol} or {Array of Symbol}s indicating the parse options
+  # extensions - An {Array of String}s indicating the extensions to use


Should be {Array of Symbol}s.

gjtorikian · 2016-09-16T21:40:16Z

lib/commonmarker/node.rb

@@ -17,11 +17,22 @@ def walk(&block)
    # Public: Convert the node to an HTML string.
    #
    # options - A {Symbol} or {Array of Symbol}s indicating the render options
+    # extensions - An {Array of String}s indicating the extensions to use


Should be {Array of Symbol}s.

gjtorikian · 2016-09-16T21:41:02Z

test/test_extensions.rb

+    assert_raises(TypeError) { CommonMarker.render_html(@markdown, :default, "nope") }
+    assert_raises(TypeError) { CommonMarker.render_html(@markdown, :default, ["table"]) }
+    assert_raises(ArgumentError) { CommonMarker.render_html(@markdown, :default, %i[table bad]) }
+  end


Love the error cases--thank you!

gjtorikian · 2016-09-16T21:44:57Z

test/test_extensions.rb

+
+    none_in_use = CommonMarker.render_html(@markdown, :default, %i[table strikethrough])
+    refute none_in_use.include?("| a")
+    refute none_in_use.include?("~~hi~~")


Could we get some assertions on the expected HTML, for example, <strike>hi</strike> or <td>a</td>? (For these, and the tests above.)

Also, could you demonstrate what happens with something like, asterisks in a table cell? github-markdown does support this at the moment.

Can do! I had left these out as cmark itself tests the cases, but I think it makes sense to assert they're working properly here too.

gjtorikian · 2016-09-16T21:45:23Z

This is looking goooood! I left a few more comments. Thanks for your patience.

kivikakk · 2016-09-17T10:26:55Z

Fixed up. And not at all, thanks for your attention! :)

gjtorikian · 2016-09-18T18:03:01Z

Shipping as 0.11.0!

Yuki Izumi added 9 commits September 1, 2016 16:47

Build extensions branch with extensions

6c67671

Add CommonMarker.extensions

6fa2772

Add extensions to all parse points

0e12210

Use extensions in to_html

c5c5931

Add --list-extensions, doc.

523e4ef

Add to_commonmark.

391e68b

Use newest cmark

7e1ce68

Use newest cmark

8eab067

Newest cmark

fc284b8

Clean up; latest cmark

e0b16af

kivikakk changed the title ~~[WIP] cmark extensions~~ cmark extensions Sep 14, 2016

Yuki Izumi added 10 commits September 15, 2016 14:44

Update cmark to https://github.com/kivikakk/cmark/commit/250baa4dcaaa…

a107ea8

…f54484bbb03423f3dfc0b2f86080

Update script/update_submodules to show true source

98909db

Latest cmark

5544b54

Windows build changes

cc14e1a

Don't ask, don't tell

2809ea7

We get conflicts in msxml.h if we don't do this. It looks like the CMARK_ prefix on our enum is getting ignored by the compiler somehow ...?!

Use MSYS Makefiles generator in mingw32

9557f9e

This at least has it compiling with RubyInstaller + DevKit.

Add appveyor.yml

7e9bcd7

Grab submodules

3840e6f

Windows encoding fun. One assertion left.

9db2029

And done! <3

9277097

gjtorikian reviewed Sep 16, 2016

View reviewed changes

kivikakk mentioned this pull request Sep 17, 2016

fix remaining markdown issues lobsters/lobsters#288

Closed

Yuki Izumi added 2 commits September 17, 2016 19:47

Minor fixes.

cc46399

Expand tests.

a75cf51

gjtorikian merged commit 15ba18b into gjtorikian:master Sep 18, 2016

kivikakk deleted the kivikakk/extensions branch September 19, 2016 02:47

gjtorikian mentioned this pull request Oct 11, 2016

Revert "cmark extensions" #29

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmark extensions #28

cmark extensions #28

kivikakk commented Sep 13, 2016 •

edited

Loading

gjtorikian commented Sep 13, 2016

kivikakk commented Sep 14, 2016

kivikakk commented Sep 14, 2016

kivikakk commented Sep 14, 2016

kivikakk commented Sep 15, 2016

gjtorikian commented Sep 16, 2016

gjtorikian Sep 16, 2016

gjtorikian Sep 16, 2016

gjtorikian Sep 16, 2016

gjtorikian Sep 16, 2016

gjtorikian Sep 16, 2016

gjtorikian Sep 16, 2016

kivikakk Sep 17, 2016

gjtorikian commented Sep 16, 2016

kivikakk commented Sep 17, 2016

gjtorikian commented Sep 18, 2016

cmark extensions #28

cmark extensions #28

Conversation

kivikakk commented Sep 13, 2016 • edited Loading

gjtorikian commented Sep 13, 2016

kivikakk commented Sep 14, 2016

kivikakk commented Sep 14, 2016

kivikakk commented Sep 14, 2016

kivikakk commented Sep 15, 2016

gjtorikian commented Sep 16, 2016

gjtorikian Sep 16, 2016

Choose a reason for hiding this comment

gjtorikian Sep 16, 2016

Choose a reason for hiding this comment

gjtorikian Sep 16, 2016

Choose a reason for hiding this comment

gjtorikian Sep 16, 2016

Choose a reason for hiding this comment

gjtorikian Sep 16, 2016

Choose a reason for hiding this comment

gjtorikian Sep 16, 2016

Choose a reason for hiding this comment

kivikakk Sep 17, 2016

Choose a reason for hiding this comment

gjtorikian commented Sep 16, 2016

kivikakk commented Sep 17, 2016

gjtorikian commented Sep 18, 2016

kivikakk commented Sep 13, 2016 •

edited

Loading