Allow for variables to be passed as filter args #50
Conversation
This commit adds support for passing variables as arguments to filter. It changes the FilterPrototype type to contain a list of VarOrVal instances rather than just Value instances. When filters are being applied, each VarOrVal instance is "unwrapped" before being given to the filter invocation. For VarOrVal::Val this means just extracting the inner Value. For VarOrVal::Var it means looking up the variable value. This fixes cobalt-org#49
| x => { | ||
| return Error::parser("a comma or a pipe", Some(x)); | ||
| } | ||
| } | ||
| comma_previous = false; | ||
| first = false; |
There was a problem hiding this comment.
I don't really like this piece.
Essentially, a test was failing because instead of raising an error for a missing comma or pipe, the parser was eating the next identifier, producing the wrong result.
To get around this, I'm keeping track of whether or not this is the first time through the list, and whether the previous token was a comma. If a comma was not directly before the new Identifier, and its not the first time through the loop, then we raise an error. This fixes the test.
A better way to do this would probably be to look ahead after parsing the Identifier and check if the next token is either a comma or a pipe. What do you think? Any thoughts / suggestions?
Let me know if you need me to clarify. If you want to see the failing test, just revert the second commit in this PR.
There was a problem hiding this comment.
Mmh the original code seems generally problematic since we just swallow any value without looking at commas in between. As far as I understand the Liquid "spec" there should always be a comma or a pipe after any filter argument.
The smartest thing to do might be to add a peek after the match that checks if we have either a comma or a pipe after our argument and fail if we don't. If there is a comma we can consume it and continue and if there's a Pipe we can just break. Then you'll obviously need to remove the Comma match clause.
Do you think that'll work?
There was a problem hiding this comment.
I think so - will give it a shot and update!
|
Will update to fix the fmt errors. Let me know what you think re the above comment! |
| entry = try!(f(&entry, &filter.arguments)); | ||
|
|
||
| let mut arguments = Vec::new(); | ||
| for arg in filter.arguments.iter() { |
There was a problem hiding this comment.
I think the iter can be inferred here
There was a problem hiding this comment.
Was getting errors when I removed the iter. Changed it to loop over &filter.arguments instead.
|
Hey, thanks so much for the PR! I left some comments. Cheers |
|
Looks great! 👍 One thing before we can merge: you accidentally committed |
|
Whoops, sorry! Didn't realise I added that. Will update in the morning when I get back to my laptop. |
|
Ok - updated! |
|
Great job! Thanks! 👍 |
This commit adds support for passing variables as arguments to filter.
It changes the FilterPrototype type to contain a list of VarOrVal
instances rather than just Value instances.
When filters are being applied, each VarOrVal instance is "unwrapped"
before being given to the filter invocation. For VarOrVal::Val this
means just extracting the inner Value. For VarOrVal::Var it means
looking up the variable value.
This fixes #49