Support parameters for liquid include tags #876

Closed
wants to merge 20 commits into
from

Projects

None yet

5 participants

@maul-esel

My try at issue #848. It adds an optional, comma-separated parameter list to the include tag.

Please bear with me, as this is my first ruby.

@maul-esel

I "tested" it with some example includes, but I gonna some need help on the "real" tests, please.

@parkr
Jekyll member

Take a look at the test/test_tags.rb file. Should be pretty straight-forward in terms of writing tests for this. Essentially, you'll want to write some Liquid, write the expected output and ensure they actual output and the expected output are the same. :-)

@maul-esel maul-esel Update 2012-07-01-templates.md
Document parameters for Liquid "include" tag
d14b1e7
@paulmsmith

Amazing. Can't wait to try this assuming all is well. :)

@maul-esel maul-esel Add tests for include tags with parameters
Add one file to source/_includes for this purpose.

All tests still pass.
661287f
@maul-esel

Let's see:

  • Code modified
  • updated docs (hope I didn't mess up the strange syntax in that file)
  • tests written (hope they're OK, otherwise let me know)
  • ensured all tests are still passing

Anything else?

@paulmsmith: me too 😄

@parkr
Jekyll member

Please write features tests for this (cucumber) – it better resembles what actually happens when Jekyll is run.

@maul-esel

I fear that's way over my head at the moment, sorry. If I read the existing code in features/, I understand nearly nothing.

I hope this feature still somehow can make it.

@maul-esel

I might give it a try later this week though.

@parkr
Jekyll member

Cucumber is pretty nifty. Basically, we define steps based on some regex and English sentence/clause. Each "scenario" is matched against one step definition and executed accordingly. We thus have the power to execute commands in the shell in the same way we'd expect a user to and test the actual file output. It's awesome!

I'd ask you to create a new include.feature document inside the ./features directory. Take a look at pagination.feature for inspiration! Also, feel free to define new steps in jekyll_steps.rb if one isn't provided for you already :-)

@maul-esel

OK. What about the existing {% include %} tests in create_sites.feature, should I move them?

@parkr parkr commented on the diff Mar 21, 2013
features/step_definitions/jekyll_steps.rb
@@ -145,6 +145,10 @@
assert_match Regexp.new(text), File.open(file).readlines.join
end
+Then /^I should not see "(.*)" in "(.*)"$/ do |text, file|
+ assert_no_match Regexp.new(text), File.open(file).readlines.join
@parkr
parkr Mar 21, 2013

Why not just File.read(file)?

@maul-esel
maul-esel Mar 21, 2013

I just copied it from the above step definition. Should I change both?

@mattr-
mattr- Mar 27, 2013

Once this is merged into master, then I would entertain another pull request that changes both, but I think it's ok for this PR

@maul-esel

Let me know if there's anything else I should do to get this merged.

@parkr
Jekyll member

Sorry I forgot to update you guys. I don't think @mojombo responds to his GitHub notifications very frequently, so I sent him an email about this a couple days ago but haven't heard back yet.

@mojombo

This is a really awesome idea. It has the power to make Includes much more powerful. I am a bit worried about the syntax for specifying parameters, though. It feels a bit like a hack at the moment. The use of commas feels very un-Liquid to me. The treatment of whitespace is hard to intuit from just seeing it. I think I'd prefer something more unambiguous, as suggested by #848:

{% include thing.html size="big" color="bright red" %}
{% include blarg.html name="Tom \"Dubs\" Preston-Werner" %}

A space delimited list of PARAM=VALUE pairs where PARAM is the variable name (no spaces allowed) and VALUE is quoted string where spaces are allowed. Quotes inside the value must be escaped.

This is totally unambiguous and feels more like Liquid to me (also familiar to anyone that knows HTML). It's more complex to implement, but I think it would prevent confusion and usage errors.

maul-esel added some commits Apr 7, 2013
@maul-esel maul-esel fix param parsing as suggested by @mojombo
This changes the syntax: instead of comma-separated,
params are now separated by spaces and the values are
enclosed in quotes.
7811409
@maul-esel maul-esel fix tests and cucumber for new param syntax d4f80e8
@maul-esel maul-esel add an extra feature step for special param values 258ae44
@maul-esel maul-esel fix indentation quirks bc4a9a5
@maul-esel maul-esel once more fix indentation 09d97fd
@maul-esel maul-esel fix include tag for Ruby 1.8.7
String[fixnum] returns the character code in Ruby <= 1.8.7.
Circumvent that by using String[fixnum, fixnum = 1] instead
which always returns a string.
d224eff
@maul-esel

Cucumber won't tell me what's wrong in Ruby 1.8.7 - any idea?

maul-esel added some commits Apr 7, 2013
@maul-esel maul-esel one more fix for Ruby 1.8.7 b4d5672
@maul-esel maul-esel fix cucumber: cannot expect a certain order of params
At least for Ruby <= 1.8.7, hashes are not sorted. Until
now a certain order of params was expected.
c2a5f51
@maul-esel

Fixed the code for Ruby 1.8.7.

@parkr: However, now it seems cucumber fails for 1.9.2 - I'm pretty sure that's not a jekyll fault. Might have to do with the update from 1.2.3 (shows deprecation warnings, but works) to 1.2.4 (no warnings, but fails AFTER all features).

@maul-esel

Unless you want some more features (like single-quote support?) or you find something I should fix, I'd say this should be done.

@paulmsmith

Any news on when this might make it into Jekyll?

@mattr-
Jekyll member

@paulmsmith After 1.0 is released.

@mattr- mattr- commented on an outdated diff Apr 18, 2013
lib/jekyll/tags/include.rb
@@ -1,9 +1,69 @@
module Jekyll
module Tags
class IncludeTag < Liquid::Tag
- def initialize(tag_name, file, tokens)
+ def initialize(tag_name, params, tokens)
@mattr-
mattr- Apr 18, 2013

Could you change the name of the second parameter ? params is also a method that you added and having a variable named params along with a method named params is confusing.

@maul-esel

This will need some more work, mostly due to the docs restructuring. Just let me know when you plan to merge this, and I'll resubmit it in a new, clean PR.

@parkr
Jekyll member

I'd love to merge for 1.1 in a week or two. Thoughts, @mattr-?

@paulmsmith

Exciting! Modular front-end development will be 1000x easier in Jekyll. Can't wait!

@parkr
Jekyll member

@mattr- - what do you think of this?

@mattr-
Jekyll member

👍 :shipit:

@parkr
Jekyll member

@maul-esel Thanks so much for your hard work on here. Would you mind rebasing on the current master? If not, I can try to fumble through it but I don't want to ruin your beautifully-written code by a botched merge. Let me know! ❤️

@maul-esel

Working on it.

@parkr
Jekyll member

Going to close this in favour of its replacement, #1204.

@parkr parkr closed this Jun 11, 2013
@maul-esel maul-esel deleted the maul-esel:include-params branch Jun 11, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment