Support parameters for liquid include tags [take 2] #1204

Merged
merged 18 commits into from Jul 9, 2013

Projects

None yet

6 participants

@maul-esel
Contributor

This is a resubmission of #876 due to the heavy changes that have been made to master since it was submitted.

This PR adds the ability to send parameters to includes:

{% include foo.md bar="baz" %}

It can be used like this:

# in _includes/foo.md
{{ include.bar }} # outputs "baz" (without the quotes)
@parkr
Member
parkr commented Jun 11, 2013

Thank you!! Sorry to ask yet another thing of you but would you mind writing a quick comment outlining exactly how this works from a user standpoint?

@parkr parkr commented on the diff Jun 11, 2013
features/include_tag.feature
@@ -0,0 +1,30 @@
+Feature: Include tags
+ In order to share their content across several pages
+ As a hacker who likes to blog
+ I want to be able to include files in my blog posts
+
+ Scenario: Include a file with parameters
+ Given I have an _includes directory
+ And I have an "_includes/header.html" file that contains "<header>My awesome blog header: {{include.param}}</header>"
+ And I have an "_includes/params.html" file that contains "Parameters:<ul>{% for param in include %}<li>{{param[0]}} = {{param[1]}}</li>{% endfor %}</ul>"
@parkr
parkr Jun 11, 2013 Member

What if we made it so that we had this:

{% for param in include %}
{{ param.key }} = {{ param.value }}
{% endfor %}

I kind of like named accessors more than index accessors.

@parkr
parkr Jun 11, 2013 Member

Ok cool :) Thanks!

@maul-esel
Contributor

You can write your {% include %} tags like before, so all old things should still work.

But if, for a particular include, you want some extra variables available depending on where they are included from, you add parameters to the tag, like {% include param='value' %}.

Inside the include, you can then access these from the liquid variable include. You can for example use this for navigations (pass the "current" section and mark it) or conditional styling (pass the CSS class to use in the include) etc.

@parkr parkr and 2 others commented on an outdated diff Jun 11, 2013
lib/jekyll/tags/include.rb
+ last_space = last_quote = pos = 0
+ last_key = nil
+ in_quotes = false
+ @params = {}
+
+ if !(/^(\s*\b\S+="(?:\\"|.)*?")*\s*$/ =~ markup)
+ raise SyntaxError.new <<-eos
+Syntax error for 'include' while parsing the following markup:
+
+ #{markup}
+
+Valid syntax: include param="value"
+eos
+ end
+
+ while pos = markup.index(/[=\"\s]/, pos)
@parkr
parkr Jun 11, 2013 Member

Why not use regexp for this?

http://rubular.com/r/FbVfmDUIU6

@maul-esel
maul-esel Jun 12, 2013 Contributor

😲 How did you come up with this? I tried, but couldn't think of any regex that does the job.

@maul-esel
maul-esel Jun 12, 2013 Contributor

One thing though: Your regex supports both single and double quote, but a double quote can terminate a string started with single quote. This leads to problems with code like the following:

param='and" dg=' abc="d"

With your current regex, the dg=' portion is leftover, though I'd expect it to be part of param.

Do you have a fix for this, god of regex? Or drop single-quote support?

@parkr
parkr Jun 12, 2013 Member

What do you think of this?

http://rubular.com/r/6P8aGivX9g

@maul-esel
maul-esel Jun 12, 2013 Contributor

That seems to fail on your other test data - it reads everything as one param.

@parkr
parkr Jun 12, 2013 Member

good catch.

Do you think it's fair to disallow = characters in the param value? That would fix that regexp.

http://rubular.com/r/eOkivPXF7Q

@maul-esel
maul-esel Jun 12, 2013 Contributor

Your choice. If you think it has to be a regex, and nobody finds one that suits all needs, that's probably the way to go - or let users escape the =, if that works as well.
I'm sorry I'm not of big help with regex.

@parkr
parkr Jun 12, 2013 Member

No it's totally cool! I was just thinking that regexp would be cleaner. :)

What would be a use-case for sending the equals in a value that you couldn't do by using keys and values? I can't think of anything at the moment.

@maul-esel
maul-esel Jun 12, 2013 Contributor

From the top of my head - me neither.

@parkr
parkr Jun 12, 2013 Member

@paulmsmith You seem pretty excited about this. Can you come up with a use-case for a param whose value includes the = character? I can't think of anything.

@paulmsmith
paulmsmith Jun 12, 2013

If I'm understanding you chaps correctly, then a use-case might just be a string of copy text. For example I could want to do:

{% include headline='Jekyll include parameters are here' strapline='Using them = EPIC WIN!' %} 

and from within the include:

<div class="somemodule">
<h1>{{ include.headline }}</h1>
<p>{{ include.strapline }}</p>
</div>

I'd expect that to output:

<div class="somemodule">
<h1>Jekyll include parameters are here</h1>
<p>Using them = EPIC WIN!</p>
</div>

Hope that makes sense.

@maul-esel
maul-esel Jun 12, 2013 Contributor

That's the only thing I could think about as well. On the other side, most cases like that can be solved by using an HTML entity for the equal sign - we'd just need to document it.

@parkr
parkr Jun 12, 2013 Member

So, guess what! @imathis (the true Regexp god) fixed everything: http://rubular.com/r/kmNhn2806o

@maul-esel
maul-esel Jun 13, 2013 Contributor

Impressive! And indeed, the regex shortens the code immensely - definitly worth it.

@parkr parkr and 1 other commented on an outdated diff Jun 11, 2013
site/docs/templates.md
@@ -192,6 +192,18 @@ Jekyll expects all include files to be placed in an `_includes` directory at the
root of your source directory. This will embed the contents of
`<source>/_includes/sig.md` into the calling file.
+You can also pass parameters to an include:
+
+{% highlight ruby %}
+{{ "{% include sig.textile param=value " }}%}
+{% endhighlight %}
@parkr
parkr Jun 11, 2013 Member

Let's use {% raw %} blocks in here instead of that weird {{ "{% syntax.

@maul-esel
maul-esel Jun 12, 2013 Contributor

Done.

@paulmsmith

You guys are awesome!

@maul-esel maul-esel and 1 other commented on an outdated diff Jun 14, 2013
lib/jekyll/tags/include.rb
+ separator = markup.index(' ')
+ @file = markup[0..separator].strip
+ parse_params(markup[separator..-1])
+ else
+ @file = markup
+ end
+ end
+
+ MATCHER = /(\w+)=(?:"([^"\\]*(?:\\.[^"\\]*)*)"|'([^'\\]*(?:\\.[^'\\]*)*)')/
+
+ def parse_params(markup)
+ @params = {}
+ pos = 0
+
+ # ensure the entire markup string from start to end is valid syntax, and params are separated by spaces
+ full_matcher = Regexp.compile('\A\s*(?:(?<=\s|\A)' + MATCHER.to_s + '\s*)*\z')
@maul-esel
maul-esel Jun 14, 2013 Contributor

I used this to make sure the entire markup is valid, not only portions of it. However, 1.8.7 doesn't like look-behind (?<=...) - any advice?

@parkr
parkr Jun 15, 2013 Member

Hm... not sure. What is the look-behind necessary for?

@maul-esel
maul-esel Jun 15, 2013 Contributor

To make sure there's a space between the individual params. I could solve this issue by using a look-ahead at the end of the pattern instead.

@maul-esel
Contributor

OK, the tests finally pass. Anything else left to do?

@imathis

What if we used this regex instead. ([\w-]+)\s*=\s*(?:"([^"\\]*(?:\\.[^"\\]*)*)"|'([^'\\]*(?:\\.[^'\\]*)*)'|([\w\.-]+)) that would add a few new options.

  1. Supports spaces around the equals sign
  2. Supports variable names with a dash (like YAML does).
  3. Supports existing variables in match[4](see next comment)
@imathis

If you set @markup = markup then you can move this regex parsing into the render method. Then using context, you can assign include variables using values from variables in the context. Using the new regex I commented about above, you'll have match[4] which will be an unquoted variable name derived from a match on something like foo=bar.baz. Then you can do the following:

elsif match[4]
  value = context[match[4]]
end

This will allow you to do something like this:

{% for post in site.posts reverse %}
  {% capture post_title %}{{ post.title | titlecase }}{% endcapture %}
  {% include post.html title=post_title %}
{% endfor %}

Then you can reference {{ include.title }} in your post template partial. By allowing variables as well as strings, template partials have to rely less on conditionals and global variables and aren't restricted to static strings like the current proposal would suggest.

@imathis
Contributor
imathis commented Jun 15, 2013

Oh, if you want to see this new regex in action, here's the rubular.

@maul-esel
Contributor

Very interesting, thanks. I considered supporting variables, just didn't know how to. Unless @parkr or @mattr- have a different opinion, I'm happy to include it.

@mattr- mattr- and 1 other commented on an outdated diff Jun 22, 2013
lib/jekyll/tags/include.rb
super
- @file = file.strip
+ markup.strip!
+ if markup.include?(' ')
+ separator = markup.index(' ')
+ @file = markup[0..separator].strip
+ parse_params(markup[separator..-1])
+ else
+ @file = markup
+ end
+ end
+
+ MATCHER = /(\w+)=(?:"([^"\\]*(?:\\.[^"\\]*)*)"|'([^'\\]*(?:\\.[^'\\]*)*)')/
@mattr-
mattr- Jun 22, 2013 Member

I'd prefer if the constant was at the top but I'm willing to be flexible.

@parkr do you have a preference here?

@parkr
parkr Jun 22, 2013 Member

I also prefer the top if that's ok :)

@mattr-
Member
mattr- commented Jun 22, 2013

I'm cool with supporting variables.

@parkr @mojombo Anything you're aware of that keeps us from supporting variables as in @imathis' example?

@parkr
Member
parkr commented Jun 22, 2013

@mattr- @mojombo I think supporting extraction of variable values if the string input is a variable is a good idea.

@imathis also suggested we try something more direct (albeit Rails-like) with something like this:

{% include post.html locals: { author_class: "blue icon-notice", title: "Herein Lies the Story" } %}

So we'd take everything after .extname and parse it as JSON. It definitely takes more work in terms of styling, but also offers hierarchies and such. Might very well be too complicated for any solution Jekyll should hold in its core, but it's a pretty neat suggestion nevertheless. I'd say go with our current implementation in order to be more designer-friendly.

maul-esel and others added some commits Jun 22, 2013
@maul-esel maul-esel move regex to the top 00ed567
@maul-esel maul-esel move parameter parsing to render time f8f6784
@imathis @maul-esel imathis Support passing Liquid variables to includes
Change the regex matching to allow Liquid variables and object fields
to be passed to the include. Use the render context to retrieve the
variable value. Also, relax syntax checks by allowing surrounding spaces
and dashes in parameter names.
2b01b06
@maul-esel maul-esel Add a cucumber test for passing variables 656dcca
@maul-esel
Contributor

Adjusted according to @imathis' suggestion, moved the regex up (@mattr-).

@bcomnes
Contributor
bcomnes commented Jun 30, 2013

This seems similar to the extended include tag in shopify: {% include 'foo' with 'bar' %}

I can't seem to get that to work in Jekyll though, and this PR looks more useful. 👍 Can't wait to get my hands on it.

@paulmsmith

Sorry to push. What's the latest on this @imathis @parkr ? Going to be in Jekyll one day soon?

@parkr parkr and 1 other commented on an outdated diff Jul 4, 2013
lib/jekyll/tags/include.rb
+ full_matcher = Regexp.compile('\A\s*(?:' + MATCHER.to_s + '(?=\s|\z)\s*)*\z')
+ if not markup =~ full_matcher
+ raise SyntaxError.new <<-eos
+Invalid syntax for include tag:
+
+ #{markup}
+
+Valid syntax:
+
+ {% include file.ext param='value' param2="value" %}
+
+eos
+ end
+
+ while match = MATCHER.match(markup) do
+ markup = markup[match.end(0)..-1]
@parkr
parkr Jul 4, 2013 Member

in practice, i think this will only ever execute once. @imathis's pattern above seems to encompass the entire call, no?

@maul-esel
maul-esel Jul 4, 2013 Contributor

No. If you look at the rubular he linked, there are several matches, so this should execute for each of them.

@parkr parkr commented on an outdated diff Jul 5, 2013
lib/jekyll/tags/include.rb
super
- @file = file.strip
+ markup.strip!
+ if markup.include?(' ')
+ separator = markup.index(' ')
+ @file = markup[0..separator].strip
+ @params = markup[separator..-1]
@parkr
parkr Jul 5, 2013 Member
@file, @params = markup.split(' ', 2)

should work :)

@parkr
parkr Jul 5, 2013 Member

and it applies to both with and without params, so both of these work:

# No params
@markup = "some_fun_thing.html"
@file, @params = markup.split(" ", 2)
@file # => "some_fun_thing.html"
@params # => nil

# With params
@markup = "some_amazing_thing.html name='henry' title='king'"
@file, @params = markup.split(" ", 2)
@file # => "some_amazing_thing.html"
@params # => "name='henry' title='king'"
@parkr parkr commented on an outdated diff Jul 5, 2013
lib/jekyll/tags/include.rb
+ if markup.include?(' ')
+ separator = markup.index(' ')
+ @file = markup[0..separator].strip
+ @params = markup[separator..-1]
+ else
+ @file = markup
+ end
+ end
+
+ def parse_params(markup, context)
+ params = {}
+ pos = 0
+
+ # ensure the entire markup string from start to end is valid syntax, and params are separated by spaces
+ full_matcher = Regexp.compile('\A\s*(?:' + MATCHER.to_s + '(?=\s|\z)\s*)*\z')
+ if not markup =~ full_matcher
@parkr
parkr Jul 5, 2013 Member

unless is a bit more idiomatic here :)

@parkr
Member
parkr commented Jul 7, 2013

@mattr-, would you please take another look at this? Looks pretty ready to me.

@mattr- mattr- commented on an outdated diff Jul 8, 2013
lib/jekyll/tags/include.rb
super
- @file = file.strip
+ @file, @params = markup.strip.split(' ', 2);
+ end
+
+ def parse_params(markup, context)
+ params = {}
+ pos = 0
@mattr-
mattr- Jul 8, 2013 Member

I think we can remove the pos variable. I'm not seeing it used anywhere.

@mattr-
Member
mattr- commented Jul 8, 2013

👍 from me, even with the comment above. :shipit:

@mattr- mattr- commented on an outdated diff Jul 8, 2013
lib/jekyll/tags/include.rb
super
- @file = file.strip
+ @file, @params = markup.strip.split(' ', 2);
+ end
+
+ def parse_params(markup, context)
+ params = {}
+ pos = 0
+
+ # ensure the entire markup string from start to end is valid syntax, and params are separated by spaces
@mattr-
mattr- Jul 8, 2013 Member

Looking at this again, the comment tells me that this should be in a separate method, something like below, perhaps?

def validate_syntax
  full_matcher = Regexp.compile('\A\s*(?:' + MATCHER.to_s + '(?=\s|\z)\s*)*\z')
  unless markup =~ full_matcher
    raise SyntaxError.new <<-eos
Invalid syntax for include tag:

  #{markup}

Valid syntax:

  {% include file.ext param='value' param2="value" %}

eos
  end
end

and then you can change the parse_params method to be like so:

def parse_params(markup, context)
  params = {}

  validate_syntax

  ...
end

Thoughts?

@maul-esel maul-esel more code improvements
Remove unused variable, extract validation to method (@mattr-).
Do not require markup to be passed to parse_params as argument.
f72365d
@parkr parkr commented on an outdated diff Jul 8, 2013
lib/jekyll/tags/include.rb
+ def parse_params(context)
+ validate_syntax
+
+ params = {}
+ markup = @params
+
+ while match = MATCHER.match(markup) do
+ markup = markup[match.end(0)..-1]
+
+ if match[2]
+ value = match[2].gsub(/\\"/, '"')
+ elsif match[3]
+ value = match[3].gsub(/\\'/, "'")
+ elsif match[4]
+ value = context[match[4]]
+ end
@parkr
parkr Jul 8, 2013 Member

what do you think about having this block return the value and setting value outside the block?

@maul-esel
Contributor

Despite the build failure, this actually runs fine.

@parkr parkr merged commit 08f6f3c into jekyll:master Jul 9, 2013

1 check passed

default The Travis CI build passed
Details
@parkr parkr added a commit that referenced this pull request Jul 9, 2013
@parkr parkr Update history to reflect merge of #1204. 42ac16b
@parkr
Member
parkr commented Jul 9, 2013

MERGED!! Thanks so much for your hard work and patience with us on this @maul-esel and props to @imathis for crazy amazing Regexp-fu.

@mattr-
Member
mattr- commented Jul 9, 2013

🎉 🎆

@maul-esel maul-esel deleted the maul-esel:include-params2 branch Jul 10, 2013
@paulmsmith

Awesome!! Thanks for all the work @maul-esel, @parkr, @imathis and chums! Looking forward to using it!!

@mattr-
Member
mattr- commented Jul 10, 2013

@maul-esel Great work! Thanks so much for sticking with this through all the various revisions and comments.

@maul-esel
Contributor

Happy to help this awesome project grow and develop further. And thanks to you both for bringing it back to life❗️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment