search

minad / olelo

Wiki with git backend
MIT License
241 stars 44 forks source link

Markdown tables not working #94

Open candlerb opened 11 years ago

candlerb commented 11 years ago

If I create a Markdown page containing a table, it doesn't render properly.

It looks like an option needs to be passed to Tilt and/or Redcarpet to enable the table extension (aside: there are other Markdown extensions I'd like to turn on). However I can't see how to do this in config.yml or elsewhere.

candlerb commented 11 years ago

I am now running the git version - same problem.

Digging deeper, here's what I've found.

  1. Redcarpet has two different sets of options. There are options to the Markdown parser (Redcarpet::Markdown.new), such as :tables=>true. But there are also options to the HTML renderer (Redcarpet::Render::HTML.new), such as :filter_html=>true
  2. There is Tilt sitting in between. AFAICS, options given to Tilt are given to the Markdown parser: @engine = ::Redcarpet::Markdown.new(generate_renderer, options)
  3. Olelo doesn't seem to pass any options to Tilt
  4. Olelo turns symbol keys into string keys (WithIndifferentAccess), but then Tilt calls :to_hash on the options object, so the 'indifferent' nature of the keys is lost. I'd say that's arguably a misfeature of Tilt.

The following proof-of-concept allows Markdown tables to work:

diff --git a/config/aspects.rb b/config/aspects.rb
index 70f4366..0972b68 100644
--- a/config/aspects.rb
+++ b/config/aspects.rb
@@ -189,7 +189,7 @@ aspect :page do
   filter do
     editsection do
       remove_comments.tag_shortcuts.markdown_nowiki
-      tag(disable: 'html:*') { markdown! }
+      tag(disable: 'html:*') { markdown!(:tables=>true) }
     end
     fix_image_links.toc
     interwiki(map: interwiki_map).link_classifier
diff --git a/plugins/filters/tilt.rb b/plugins/filters/tilt.rb
index 4ad4ff5..2ee78ad 100644
--- a/plugins/filters/tilt.rb
+++ b/plugins/filters/tilt.rb
@@ -3,7 +3,9 @@ require 'tilt'

 class TiltFilter < Filter
   def filter(context, content)
-    ::Tilt[name].new { content }.render
+    repair_options = {}
+    options.each { |k,v| repair_options[k.to_sym] = v }
+    ::Tilt[name].new(nil, 1, repair_options) { content }.render
   end
 end

However it might be better, rather than forcing people to edit config/aspects.rb directly, to have some way of setting renderer options in config.yml

Aside: it looks like Redcarpet only supports PHP-style pipe tables, not the other table formats supported by e.g. pandoc.

Aside 2: Tilt doesn't appear to handle the :escape_html option when using Redcarpet.

minad commented 11 years ago

Content preview: Maybe we should enable some options by default or by using the *.yml file. Would you mind creating a patch? I wonder if it is necessary to transform the Hash keys. Doesn't Tilt accept the indifferentaccesshash? [...]

Content analysis details: (-2.9 points, 5.0 required)

pts rule name description

0.0 URIBL_BLOCKED ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [URIs: github.com] -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000]

Maybe we should enable some options by default or by using the *.yml file. Would you mind creating a patch? I wonder if it is necessary to transform the Hash keys. Doesn't Tilt accept the indifferentaccesshash?

I am now running the git version - same problem.

Digging deeper, here's what I've found.

*

Redcarpet has two different sets of options. There are options to the Markdown parser (Redcarpet::Markdown.new), such as :tables=>true. But there are also options to the HTML renderer (Redcarpet::Render::HTML.new), such as :filter_html=>true *

There is Tilt sitting in between. AFAICS, options given to Tilt are given to the Markdown parser: @engine = ::Redcarpet::Markdown.new(generate_renderer, options) *

Olelo doesn't seem to pass any options to Tilt *

Olelo turns symbol options into string options, due to WithIndifferentAccess

The following proof-of-concept allows Markdown tables to work:

diff --git a/config/aspects.rb b/config/aspects.rb index 70f4366..0972b68 100644 --- a/config/aspects.rb +++ b/config/aspects.rb @@ -189,7 +189,7 @@ aspect :page do filter do editsection do remove_comments.tag_shortcuts.markdown_nowiki

  • tag(disable: 'html:*') { markdown! }

  • tag(disable: 'html:*') { markdown!(:tables=>true) } end fix_image_links.toc interwiki(map: interwiki_map).link_classifier diff --git a/plugins/filters/tilt.rb b/plugins/filters/tilt.rb index 4ad4ff5..2ee78ad 100644 --- a/plugins/filters/tilt.rb +++ b/plugins/filters/tilt.rb @@ -3,7 +3,9 @@ require 'tilt'

    class TiltFilter < Filter def filter(context, content)

  • ::Tilt[name].new { content }.render
  • repair_options = {}
  • options.each { |k,v| repair_options[k.to_sym] = v }
  • ::Tilt[name].new(nil, 1, repair_options) { content }.render end end

However it might be better, rather than forcing people to edit config/aspects.rb directly, to have some way of setting renderer options in config.yml

Aside: it looks like Redcarpet only supports PHP-style pipe tables, not the other table formats supported by e.g. pandoc.

Aside 2: Tilt doesn't appear to handle the :escape_html option when using Redcarpet.

Reply to this email directly or view it on GitHub [1].

Links:

[1] https://github.com/minad/olelo/issues/94#issuecomment-23590447

candlerb commented 11 years ago

Maybe we should enable some options by default or by using the *.yml file. Would you mind creating a patch?

I was seeking your steer as how you'd like to have this done. One way is to change config/aspects.rb to do the lookup each time:

render_options = Olelo::Config['render_options'] || {}

...

tag(disable: 'html:*') { markdown!(render_options['markdown'] || {}) }

But that's a lot of repetition. Perhaps it would be better to read the global yml render options in plugins/filters/tilt.rb, and merge in any options passed to the filter.

I wonder if it is necessary to transform the Hash keys. Doesn't Tilt accept the indifferentaccesshash?

From the Tilt (1.4.1) code:

    def initialize(file=nil, line=1, options={}, &block)
      @file, @line, @options = nil, 1, {}

      [options, line, file].compact.each do |arg|
        case
        when arg.respond_to?(:to_str)  ; @file = arg.to_str
        when arg.respond_to?(:to_int)  ; @line = arg.to_int
        when arg.respond_to?(:to_hash) ; @options = arg.to_hash.dup
        when arg.respond_to?(:path)    ; @file = arg.path
        else raise TypeError
        end
      end

So, the options argument has to_hash.dup applied. But the olelo code has this in WithIndifferentAccess:

    def to_hash
      Hash.new(default).merge(self)
    end

So by this stage, it's turned back into a normal hash, and Olelo has converted symbol keys to strings :-(

candlerb commented 11 years ago

And the other thing is what to do about WithIndifferentAccess. Rather than getting Tilt to change, perhaps we can set a singleton to_hash method which just passes through the object unchanged.

candlerb commented 11 years ago

Overriding to_hash doesn't work. redcarpet is written in C and directly looks for hash keys which are symbols, rather than going via a :[] method dispatch.

static void rb_redcarpet_md_flags(VALUE hash, unsigned int *enabled_extensions_p)
{
        unsigned int extensions = 0;

        Check_Type(hash, T_HASH);

        /**
         * Markdown extensions -- all disabled by default
         */
        if (rb_hash_lookup(hash, CSTR2SYM("no_intra_emphasis")) == Qtrue)
                extensions |= MKDEXT_NO_INTRA_EMPHASIS;

        if (rb_hash_lookup(hash, CSTR2SYM("tables")) == Qtrue)
                extensions |= MKDEXT_TABLES;
... etc

Incidentally, I found what I think is a bit of an inconsistency: Olelo::Config#to_hash returns a WithIndifferentAccess object, but WithIndifferentAccess#to_hash returns a regular Hash.

minad commented 11 years ago

Content preview: I think we should do that. On 2013-08-31 11:30, Brian Candler wrote: > And the other thing is what to do about WithIndifferentAccess. Rather

than getting Tilt to change, perhaps we can set a singleton to_hash > method which just passes through the object unchanged. > > -- > Reply to this email directly or view it on GitHub [1]. > > Links: > ------ > [1] https://github.com/minad/olelo/issues/94#issuecomment-23603559 [...]

Content analysis details: (-2.9 points, 5.0 required)

pts rule name description

0.0 URIBL_BLOCKED ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [URIs: github.com] -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000]

I think we should do that.

On 2013-08-31 11:30, Brian Candler wrote:

And the other thing is what to do about WithIndifferentAccess. Rather than getting Tilt to change, perhaps we can set a singleton to_hash method which just passes through the object unchanged.

Reply to this email directly or view it on GitHub [1].

Links:

[1] https://github.com/minad/olelo/issues/94#issuecomment-23603559

minad commented 11 years ago

Content preview: The solution with less repetition is definitely better. We should also use better defaults in some cases. On 2013-08-31 10:52, Brian Candler wrote: >> Maybe we should enable some options by default or by using the _.yml >> file. Would you mind creating a patch? > > I was seeking your steer as how you'd like to have this done. One way > is to change config/aspects.rb to do the lookup each time: > > render_options = Olelo::Config['renderoptions'] || {} > > ... > > tag(disable: 'html:') { markdown!(render_options['markdown'] || {}) > } > > But that's a lot of repetition. Perhaps it would be better to read > the > global yml render options in plugins/filters/tilt.rb, and merge in > any > options passed to the filter. > >> I wonder if it is necessary to transform the Hash keys. Doesn't Tilt >> accept the indifferentaccesshash?

From the Tilt (1.4.1) code: > > def initialize(file=nil, line=1, options={}, &block) > @file, @line, @options = nil, 1, {} > > [options, line, file].compact.each do |arg| > case > when arg.respond_to?(:to_str) ; @file = arg.to_str > when arg.respond_to?(:to_int) ; @line = arg.to_int > when arg.respond_to?(:to_hash) ; @options = arg.to_hash.dup > when arg.respond_to?(:path) ; @file = arg.path else raise TypeError > end > end > > So, the options argument has to_hash.dup applied. But the olelo code > has this in WithIndifferentAccess: > > def to_hash > Hash.new(default).merge(self) > end > > So by this stage, it's turned back into a normal hash, and Olelo has > converted symbol keys to strings :-( > > -- > Reply to this email directly or view it on GitHub [1]. > > Links: ------ > [1] https://github.com/minad/olelo/issues/94#issuecomment-23603079 [...]

Content analysis details: (-2.9 points, 5.0 required)

pts rule name description

0.0 URIBL_BLOCKED ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [URIs: github.com] -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000]

The solution with less repetition is definitely better. We should also use better defaults in some cases.

On 2013-08-31 10:52, Brian Candler wrote:

Maybe we should enable some options by default or by using the *.yml file. Would you mind creating a patch?

I was seeking your steer as how you'd like to have this done. One way is to change config/aspects.rb to do the lookup each time:

render_options = Olelo::Config['render_options'] || {}

...

tag(disable: 'html:*') { markdown!(render_options['markdown'] || {}) }

But that's a lot of repetition. Perhaps it would be better to read the global yml render options in plugins/filters/tilt.rb, and merge in any options passed to the filter.

I wonder if it is necessary to transform the Hash keys. Doesn't Tilt accept the indifferentaccesshash?

From the Tilt (1.4.1) code:

def initialize(file=nil, line=1, options={}, &block) @file, @line, @options = nil, 1, {}

[options, line, file].compact.each do |arg| case when arg.respond_to?(:to_str) ; @file = arg.to_str when arg.respond_to?(:to_int) ; @line = arg.to_int when arg.respond_to?(:to_hash) ; @options = arg.to_hash.dup when arg.respond_to?(:path) ; @file = arg.path else raise TypeError end end

So, the options argument has to_hash.dup applied. But the olelo code has this in WithIndifferentAccess:

def to_hash Hash.new(default).merge(self) end

So by this stage, it's turned back into a normal hash, and Olelo has converted symbol keys to strings :-(

Reply to this email directly or view it on GitHub [1].

Links:

[1] https://github.com/minad/olelo/issues/94#issuecomment-23603079

minad commented 11 years ago

Content preview: Ok, so at first we should fix the inconsistency and then we cannot avoid the conversion of the indifferentaccesshash to a symbol hash. Maybe add a method to indifferentaccess to_hash (returns string hash) and to_symbol_hash (returns symbol hash). [...]

Content analysis details: (-2.9 points, 5.0 required)

pts rule name description

0.0 URIBL_BLOCKED ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [URIs: github.com] -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000]

Ok, so at first we should fix the inconsistency and then we cannot avoid the conversion of the indifferentaccesshash to a symbol hash. Maybe add a method to indifferentaccess to_hash (returns string hash) and to_symbol_hash (returns symbol hash).

On 2013-08-31 21:33, Brian Candler wrote:

Overriding to_hash doesn't work. redcarpet is written in C and directly looks for hash keys which are symbols, rather than going via a :[] method dispatch.

static void rb_redcarpet_md_flags(VALUE hash, unsigned int *enabled_extensions_p) { unsigned int extensions = 0;

Check_Type(hash, T_HASH);

/**

  • Markdown extensions -- all disabled by default */ if (rb_hash_lookup(hash, CSTR2SYM("no_intra_emphasis")) == Qtrue) extensions |= MKDEXT_NO_INTRA_EMPHASIS;

    if (rb_hash_lookup(hash, CSTR2SYM("tables")) == Qtrue) extensions |= MKDEXT_TABLES; ... etc

Incidentally, I found what I think is a bit of an inconsistency: Olelo::Config#to_hash returns a WithIndifferentAccess object, but WithIndifferentAccess#to_hash returns a regular Hash.

Reply to this email directly or view it on GitHub [1].

Links:

[1] https://github.com/minad/olelo/issues/94#issuecomment-23612471

candlerb commented 10 years ago

Suggested fix: https://github.com/candlerb/olelo/tree/candlerb/tilt_options

Note, I have not changed Olelo::Config#to_hash (i.e. it still returns WithIndifferentAccess) as I don't know what depends on this.