liquid icon indicating copy to clipboard operation
liquid copied to clipboard

add fact that to_str doesn't do anything anymore for drops to the history.md

Open Hampei opened this issue 7 years ago • 4 comments

When I wrote my first drop, to_s didn't change the way a drop class rendered itself, but to_str did. In the new version it's the other way around, but I can't find anything on this in issues or commits.

So

class MyDrop < Liquid::Drop
  def to_str
    'Joe'
  end
end

t = Liquid::Template.parse("hi {{ foo }}")
t.render 'foo' => MyDrop.new

would render "hi Joe" in liquid 3 and "hi MyDrop" in liquid 4.

Also added the fact that we can't use dynamic methods starting with a number anymore (which is totally sane, but took me a bit to debug).

class MyDrop < Liquid::Drop
  def before_method(m)
    if m == '100px'
      'works'
    end
  end
end

t = Liquid::Template.parse("hi {{ foo.100px }}")
t.render 'foo' => MyDrop.new
=> "works
class MyDrop < Liquid::Drop
  def liquid_method_missing(m)
    if m == '100px'
      'works'
    end
  end
end

t = Liquid::Template.parse("hi {{ foo.100px }}")
# Liquid::SyntaxError: Liquid syntax error: Expected id but found number in "{{ foo.100px }}"
# from /Users/henk/.rvm/gems/ruby-2.4.4/gems/liquid-4.0.1/lib/liquid/parser.rb:16:in `consume'
t.render 'foo' => MyDrop.new
=> "hi MyDrop"

Hampei avatar Nov 08 '18 09:11 Hampei

This is due to https://github.com/Shopify/liquid/pull/492/commits/9f7e6011109e0f979043f89542f5bcebea51e407. I'm fairly sure breaking to_str for drops was not intended. We could solve this by having the default Liquid::Drop#to_s implementation be something like:

- self.class.to_s
+ respond_to?(:to_str) ? to_str : self.class.to_s

Or change render_node_to_output:

- node_output = node_output.is_a?(Array) ? node_output.join : node_output.to_s
+ node_output = if node_output.is_a?(Array)
+   node_output.join
+ elsif node_output.respond_to?(:to_str)
+   node_output.to_str
+ else
+   node_output.to_s
+ end

or just state the breaking change. WDYT @dylanahsmith @fw42? to_str seems like a reasonable thing to support.

pushrax avatar Nov 13 '18 18:11 pushrax

I'm onboard with changing the default Liquid::Drop#to_s method to support overriding to_str. However, I would avoid the respond_to check by doing this

-    def to_s
+    def to_str
       self.class.name
     end

+    def to_s
+      to_str
+    end

dylanahsmith avatar Nov 14 '18 01:11 dylanahsmith

Very good point, that is the more sensible way.

pushrax avatar Nov 14 '18 03:11 pushrax

@dylanahsmith All of my drops now actually look exactly like this, since only defining to_s broke some of our filters.

So shall I make a new PR for the other changes I made to the history.md?

Feel free to just copy them into master, saves me work.

Hampei avatar Nov 14 '18 07:11 Hampei