liquid
liquid copied to clipboard
add fact that to_str doesn't do anything anymore for drops to the history.md
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"
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.
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
Very good point, that is the more sensible way.
@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.