-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for as
in Render and Include tags
#1181
Conversation
lib/liquid/tags/include.rb
Outdated
@@ -16,18 +16,20 @@ module Liquid | |||
# {% include 'product' for products %} | |||
# | |||
class Include < Tag | |||
Syntax = /(#{QuotedFragment}+)(\s+(?:with|for)\s+(#{QuotedFragment}+))?/o | |||
SYNTAX = /(#{QuotedFragment}+)(\s+(?:with|for)\s+(#{QuotedFragment}+))?(\s+(?:as)\s+(#{VariableSignature}+))?/o |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why #{VariableSignature}+
and not just #{VariableSignature}
? (if we expect only one)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VariableSignature
= /\(?[\w\-\.\[\]]\)?/
only matches characters so we expect many
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually thinking now I should change this to VariableSegment
= /[\w\-]/
lib/liquid/tags/render.rb
Outdated
context.find_variable(template_name, raise_on_not_found: false) | ||
end | ||
|
||
variable = [variable] unless variable.is_a?(Array) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Protip: variable = Array(variable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said we could also maybe turn the .each
into a method, so that we can avoid making a one-item array and do something like instead:
if variable_is_a?(Array) then variable.each(&:method_name) else method_name(variable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to be fully equivalent variable = Array(variable)
. I think its treating nil
as []
however need it to be [nil]
so it loops.
I was originally going down the method path but there are so many params in this scope that would need to pass in context
, template_name
, output
, var
, partial
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Let's add for that then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure on the Rubyist view on this, but just used a lambda to group the functionality, gets around excessive params and needing to create a fake array.
@@ -16,18 +16,20 @@ module Liquid | |||
# {% include 'product' for products %} | |||
# | |||
class Include < Tag | |||
Syntax = /(#{QuotedFragment}+)(\s+(?:with|for)\s+(#{QuotedFragment}+))?/o | |||
SYNTAX = /(#{QuotedFragment}+)(\s+(?:with|for)\s+(#{QuotedFragment}+))?(\s+(?:as)\s+(#{VariableSegment}+))?/o |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh actually, could we force quotes around the variable name. Otherwise it reads like you're passing a dynamic name defined by a variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be the way it is, It's not a string it's forcing you to define the variable you want it to be. Matches up with how other tags are defined
{% render "product" product as product %}
{% for product in products %}
{% assign product = products %}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me that syntax suggests these should all be
{% render "product" product as "product" %}
{% for "product" in products %}
{% assign "product" = products %}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm but then I would think it sets a variable in the parent context. What do you not like about: {% render "product" with product as "product" %}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's just so out of place, it's the only instance where a variable name would be quoted. For me, anything inside quotes means you can have any character. In this instance, it is introducing a new concept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of the normal "(.*)"
you have a similar but different "([\w-]*)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. Good point.
This is to support a new syntax that allows
with
andfor
to be aliased to something other than the template name.Include now supports
as
Example
Usage inside
product_alias
would beRender now supports
with
,for
andas
Render now supports
with
andfor
with a major difference being thatrender
generates a new subcontext for each loop infor
. This means each iteration offor
cannot impact others, this will allow loops to beasync
in the future for performance improvements.@Shopify/guardians-of-the-liquid @samdoiron @Thibaut