Markdown tables not working

[candlerb]

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]

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]

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] #94 (comment)

[candlerb]

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]

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]

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]

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] #94 (comment)
[...]

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] #94 (comment)

[minad]

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['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] #94 (comment)
[...]

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] #94 (comment)

[minad]

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] #94 (comment)

[candlerb]

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.