variables in include tag with filters #1789

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@jens-na
jens-na commented Dec 8, 2013

Use variable {% include %} with filters
This pull request adds support for filters when using the include tag with a variable.
To be more precise:

Since PR #1495 it is possible to use variables inside an include tag, like that:

    {% for item in page.dependencies %}
        {% include {{item}} %}
    {% endfor %}

but unfortunately you cannot use filters.

With this pull reuqest you can use something like this:

    {% for item in page.dependencies %}
        {% include {{ item | append: '.html' }} %}
    {% endfor %}

This is especially useful when you want to modify the file/filename which should get included.

Possible use cases

Append .html to the file name

{% include {{ item | append: '.html' }} %}

Make sure the file name is lowercase

{% include {{ item | downcase | append: '.html' }} %}

Prepend a directory to choose another directory inside the _includes/ directory

{% include {{ item | downcase | prepend: '/dependencies/' | append: '.html' }} %}
==> ./_includes/dependencies/<item>.html
@maul-esel
Contributor

Looks great❗️ A few questions:

  • Does this also tackle the problem where something like {% include {{ var }} %} was failing (note the spaces inside the inner brackets) ? If so, could you please add a small test for that or change one of the original tests and remove the note concerning this limitation from the docs?
  • I saw you changed the error messages to use file instead of @file. Any specific reason?
  • Does it also work for literals with filters, like {{ 'myfile' | append: '.html' }} ? Not that I see urgent need for that, just asking :)
  • Might not be a bad idea to add a test with several filters, to make sure it's not accidentally broken in the future.
@jens-na
jens-na commented Dec 8, 2013

Thanks for your feedback!

I readjusted the regex which does the parsing. Now arbitary whitespace may be used inside the variable. Something like this should be possbile: http://rubular.com/r/SqZ9BbyNNl

I changed the error messages, because @file is not always the value you want to see in an error message:

file = retrieve_variable(context) || @file
# @file => "{{ item | append: '.html'}}"
# file => "myfile.html"

Example error message with @file
Liquid Exception: Included file '{{ item | append: '.html'}}' not found in '_includes' directory in _layouts/post.html
Example error message with file
Liquid Exception: Included file '<path>/_includes/myfile.html' not found in '_includes' directory in _layouts/post.html

I think the 2nd error message is more informative. What do you think?

Filtering literals like {{ 'myfile' | append: '.html' }} does not work right now. Sorry :(

I will add some tests!

@maul-esel
Contributor
  1. Cool! That was an old result of my lazyness I always wanted to remove. :) Thanks for taking care of it.
  2. I'm undecided - best would be to work around it by showing both. If only the actual path is displayed, people (especially people like me) could go all crazy about "I don't even have that kind of tag anywhere", if only the variable / filters are displayed, one has to do extra steps to output the actual value to debug that.
  3. Well, no need to be sorry. I couldn't imagine a real-world use-case anyway.
  4. 👍
@maul-esel
Contributor

As always, the regex currently fails in Ruby 1.8.7 because it can't do named groups. @parkr, @mattr- is it necessary to work around this or not as 2.0 won't be 1.8.7-compatible anyway?

@maul-esel maul-esel commented on an outdated diff Dec 8, 2013
lib/jekyll/tags/include.rb
@@ -80,9 +85,10 @@ def file_read_opts(context)
end
def retrieve_variable(context)
- if /\{\{([\w\-\.]+)\}\}/ =~ @file
- raise ArgumentError.new("No variable #{$1} was found in include tag") if context[$1].nil?
- context[$1]
+ if /(?<variable>\{\{\s*(?<name>[\w\-\.]+)\s*(\|.*)?\}\})(?<params>.*)/ =~ @file
@maul-esel
maul-esel Dec 8, 2013 Contributor

As you're using the same regex twice, you might want to move it to a constant, like VALID_SYNTAX above.

@maul-esel maul-esel and 1 other commented on an outdated diff Dec 8, 2013
lib/jekyll/tags/include.rb
@@ -80,9 +85,10 @@ def file_read_opts(context)
end
def retrieve_variable(context)
- if /\{\{([\w\-\.]+)\}\}/ =~ @file
- raise ArgumentError.new("No variable #{$1} was found in include tag") if context[$1].nil?
- context[$1]
+ if /(?<variable>\{\{\s*(?<name>[\w\-\.]+)\s*(\|.*)?\}\})(?<params>.*)/ =~ @file
+ partial = Liquid::Template.parse(@file)
+ raise ArgumentError.new("No variable #{name} was found in include tag") if context[name].nil?
@maul-esel
maul-esel Dec 8, 2013 Contributor

Do we still need this check? Or does liquid already throw an exception in the next line if there's an unknown variable?

@jens-na
jens-na Dec 8, 2013

You're right, we can remove the regex check.

@maul-esel
maul-esel Dec 8, 2013 Contributor

I didn't mean to remove the regex itself, but the raise ... if context[name].nil? line. No idea why the tests pass like this, need to look through it closer to understand why they don't fail terribly 😉

@jens-na
jens-na Dec 8, 2013

liquid seems to do nothing, if there is nothing to render. It simply returns the input. That's way nothing fails, but I will add the regex again this is a better/clearer solution.

@maul-esel maul-esel commented on an outdated diff Dec 8, 2013
lib/jekyll/tags/include.rb
@@ -79,10 +84,13 @@ def file_read_opts(context)
context.registers[:site].file_read_opts
end
- def retrieve_variable(context)
- if /\{\{([\w\-\.]+)\}\}/ =~ @file
- raise ArgumentError.new("No variable #{$1} was found in include tag") if context[$1].nil?
- context[$1]
+ # Render the variable if required
+ def render_variable(context)
+ begin
+ partial = Liquid::Template.parse(@file)
+ partial.render!(context)
+ rescue => e
+ raise IncludeTagError.new e.message, File.join(INCLUDES_DIR, @file)
@maul-esel
maul-esel Dec 8, 2013 Contributor

I discourage using IncludeTagError here, because this way jekyll reports a potential error to be in @file, while it is actually in the file where the include tag is present. Just let the usual liquid exceptions pass through without rescueing, they'll be rescued and handled properly in convertible.rb.

@mattr-
Member
mattr- commented Dec 9, 2013

I wouldn't worry about working around the named group problem.

@parkr
Member
parkr commented Dec 26, 2013

Fixed by merge of #1841.

@parkr parkr closed this Dec 26, 2013
@jens-na jens-na referenced this pull request Mar 15, 2014
Merged

Variable {% include %} #1495

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