Converters can run before liquid is evaluated [#889] #917

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants

(A response to #889)

Specified converters can now be run before liquid tags are evaluated by including the method run_before_liquid!. Tests and docs have been updated to reflect this, and the PreIdentity converter has been added as a lowest-priority catch-all for pre-liquid converters. The new workflow is:

  1. Run pre-liquid converters
  2. Evaluate liquid tags
  3. Run post-liquid (default) converters

Note that this means that posts may go through two different converters - one pre-liquid and one post-liquid.


Converters are added to the post-liquid queue by default. This means that if you don't add any pre-liquid converters, behaviour should default to standard.


Unit tests run fine on my machine, and manual tests seem to show that everything is working well. Feedback appreciated.

Owner

mattr- commented Apr 2, 2013

I'm afraid I don't understand the reason for the pre-liquid identity converter. Could you fill me in on that?

@mattr Convertible#transform assumes that Convertible#convert will return a Converter. The role of Converters::Identity is (as I see it) to be the "backup" converter, to be returned if no other converters match the file.

In this branch, every Converter subclass is designated to run either before liquid tags are evaluated, or after. By default they'll run afterwards (as was the behaviour previously). Convertible#converter now takes an argument (:before or :after, defaults to :after) and will only retrieve converters that are designed to run either before or after liquid tags are evaluated (depending on said argument). Since Convertible#transform doesn't handle nil returns from converter well, Converters::PreIdentity is the "backup" converter for the pre-liquid conversion call.

(This also suggests that maybe transform should be able to deal with nil returns from converter?)

Does this explanation make sense? If you want I can add some comments to the PreIdentity class, further expanding on its role.

Owner

mattr- commented Apr 2, 2013

This helps tons. Thanks!

@kelvinst kelvinst commented on an outdated diff Jul 23, 2013

lib/jekyll/converter.rb
@@ -44,5 +44,29 @@ def pygments_prefix
def pygments_suffix
self.class.pygments_suffix
end
+
+ # Does this converter run before or after liquid tags are evaluated?
+ #
+ # Returns true if this converter will be run before liquid is processed.
+ def self.run_before_liquid?
+ @run_before_liquid || false
@kelvinst

kelvinst Jul 23, 2013

Sorry, this expression doesn't make much sense... Think with me, @run_before_liquid will be used only if it is setted, in other words nil == @run_before_liquid, right? But, internally, ruby considers nil as false, then you can use it in boolean expressions, making only if @run_befor_liquid... Then, making that above is like make the condition if @run_before_liquid || false, that doesn't make sense too, since the value after || will be used if, and only if, the value before || is false... Understand?

This PR LGTM... 👍
But, maybe you can add a cucumber test too, since it's some kind of new feature, I don't know if only shoulda is good enough for this...

@parkr, what do you think about?

@kelvinst Just been poking around inside cucumber this evening. I'm not sure I'm clever enough to test this with cucumber - as it stands I've only introduced an "identity" pre-liquid converter. By definition that's pretty hard to test since it should leave the page exactly as it was. Not sure if it's worth introducing a pre-liquid converter just to make cucumber work?

Have fixed converter.rb and the typo @dracula2000 spotted.

@maul-esel maul-esel commented on an outdated diff Jul 25, 2013

lib/jekyll/converters/pre_identity.rb
@@ -0,0 +1,22 @@
+module Jekyll
+ module Converters
+ # The default converter to run before parsing liquid tags
+ class PreIdentity < Converter
+ safe true
+ priority :lowest
+ run_before_liquid!
@maul-esel

maul-esel Jul 25, 2013

Contributor

Why not inherit the Identity Converter and reduce this class to the line above only? Would spare some code.

Contributor

maul-esel commented Jul 25, 2013

I like this! 👍

Just some minor notes:

  1. see my inline comment above
  2. It could probably be simplified to use only the symbols (:before or :after) or only the boolean (@run_before_liquid). Both seems like a bit of duplication to me.
  3. Your last commit introduced a file that isn't meant to be there, I think 😄

Agreed with @maul-esel in the 2nd note (:

OK, I've made some changes to the Converter following @maul-esel 's comments. Currently passing unit tests but not some cucumber tests I believe. I may get time this weekend to poke around and work out exactly what's breaking.

Owner

parkr commented Aug 1, 2013

@jyruzicka Please also rebase on the current master, please :)

@jyruzicka Jan-Yves Ruzicka Allow Converters to run before liquid tags are evaluated.
Converters can now be run before liquid tags are evaluated by including the method `run_before_liquid!`. Tests and docs have been updated to reflect this, and the `PreIdentity` converter has been added as a lowest-priority catch-all.
d03421c

Incredibly, I think I managed to rebase/git push -f without breaking anything.

Owner

parkr commented Aug 4, 2013

Nice one :)

parkr closed this Mar 17, 2014

parkr reopened this Mar 22, 2014

parkr closed this Dec 29, 2014

nhoizey referenced this pull request Jul 12, 2016

Closed

Ease Liquid evaluation after/from Converter plugins #5099

3 of 4 tasks complete

jekyllbot locked and limited conversation to collaborators Feb 27, 2017

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