-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add indexed access to context values #141
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
43fddab
Add indexed access to context values
8c51d00
Fix lints and formatting
58e99fe
More informative error messages
79debf7
Add negative index, fix formatting
fa86130
Subexpression test, small fixes
3aa5feb
Fix formatting to comply rustfmt 0.8.6
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,219 @@ | ||
use Renderable; | ||
use value::Value; | ||
use context::Context; | ||
use token::Token; | ||
use token::Token::*; | ||
use error::{Error, Result}; | ||
|
||
#[derive(Debug, Clone, Eq, PartialEq)] | ||
pub struct IdentifierPath { | ||
value: String, | ||
indexes: Vec<Value>, | ||
} | ||
|
||
impl Renderable for IdentifierPath { | ||
fn render(&self, context: &mut Context) -> Result<Option<String>> { | ||
let value = context | ||
.get_val(&self.value) | ||
.ok_or_else(|| Error::Render(format!("{} not found in context", self.value)))? | ||
.clone(); | ||
|
||
let mut counter = self.indexes.len(); | ||
let result = self.indexes.iter().fold(Ok(&value), |value, index| { | ||
// go through error | ||
let value = value?; | ||
counter -= 1; | ||
match (value, index) { | ||
(&Value::Array(ref value), &Value::Num(ref x)) => { | ||
// at the first condition only is_normal is not enough | ||
// because zero is not counted normal | ||
if (*x != 0f32 && !x.is_normal()) || | ||
x.round() > (::std::isize::MAX as f32) || | ||
x.round() < (::std::isize::MIN as f32) { | ||
return Error::renderer(&format!("bad array index: '{:?}'", x)); | ||
} | ||
|
||
let idx = if *x >= 0f32 { | ||
x.round() as usize | ||
} else { | ||
value.len() - (-x.round() as usize) | ||
}; | ||
let err = || | ||
Error::Render( | ||
format!("index out of range: got '{:?}' while array len is '{:?}'", | ||
idx, | ||
value.len() | ||
) | ||
); | ||
let value = | ||
value | ||
.get(idx) | ||
.ok_or_else(err)?; | ||
Ok(value) | ||
} | ||
(&Value::Array(_), x) => { | ||
Error::renderer( | ||
&format!("bad array index type: got array indexed by '{:?}'", x) | ||
) | ||
} | ||
(&Value::Object(ref value), &Value::Str(ref x)) => { | ||
let err = || Error::Render(format!("object element '{:?}' not found", x)); | ||
let value = | ||
value | ||
.get(x) | ||
.ok_or_else(err)?; | ||
Ok(value) | ||
} | ||
(&Value::Object(_), x) => { | ||
Error::renderer( | ||
&format!("bad object index type: expected string, but got '{:?}'", x) | ||
) | ||
} | ||
(value, _) if counter == 0 => Ok(value), | ||
(value, _) => { | ||
Error::renderer( | ||
&format!("expected indexable element, but found '{:?}'", value) | ||
) | ||
} | ||
} | ||
}); | ||
|
||
result?.render(context) | ||
} | ||
} | ||
|
||
impl IdentifierPath { | ||
pub fn new(value: String) -> Self { | ||
Self { | ||
value: value, | ||
indexes: Vec::new(), | ||
} | ||
} | ||
pub fn append_indexes(&mut self, tokens: &[Token]) -> Result<()> { | ||
let rest = match tokens[0] { | ||
Dot if tokens.len() > 1 => { | ||
match tokens[1] { | ||
Identifier(ref x) => self.indexes.push(Value::Str(x.clone())), | ||
_ => { | ||
return Error::parser("identifier", Some(&tokens[0])); | ||
} | ||
}; | ||
2 | ||
} | ||
OpenSquare if tokens.len() > 2 => { | ||
let index = match tokens[1] { | ||
StringLiteral(ref x) => Value::Str(x.clone()), | ||
NumberLiteral(ref x) => Value::Num(*x), | ||
_ => { | ||
return Error::parser("number | string", Some(&tokens[0])); | ||
} | ||
}; | ||
self.indexes.push(index); | ||
|
||
if tokens[2] != CloseSquare { | ||
return Error::parser("]", Some(&tokens[1])); | ||
} | ||
3 | ||
} | ||
_ => return Ok(()), | ||
}; | ||
|
||
if tokens.len() > rest { | ||
self.append_indexes(&tokens[rest..]) | ||
} else { | ||
Ok(()) | ||
} | ||
} | ||
} | ||
#[cfg(test)] | ||
mod test { | ||
use value::Value; | ||
use parse; | ||
use Renderable; | ||
use Context; | ||
use LiquidOptions; | ||
use std::collections::HashMap; | ||
|
||
#[test] | ||
fn identifier_path_array_index() { | ||
let options = LiquidOptions::with_known_blocks(); | ||
let template = "array: {{ test_a[0] }}"; | ||
|
||
let mut context = Context::new(); | ||
let test = Value::Array(vec![Value::Str("test".to_owned())]); | ||
context.set_val("test_a", test); | ||
|
||
let template = parse(template, options).unwrap(); | ||
assert_eq!(template.render(&mut context).unwrap(), | ||
Some("array: test".to_owned())); | ||
} | ||
|
||
#[test] | ||
fn identifier_path_array_index_negative() { | ||
let options = LiquidOptions::with_known_blocks(); | ||
let template = "array: {{ test_a[-1] }}"; | ||
|
||
let mut context = Context::new(); | ||
let test = Value::Array(vec![Value::Str("test1".to_owned()), | ||
Value::Str("test2".to_owned())]); | ||
context.set_val("test_a", test); | ||
|
||
let template = parse(template, options).unwrap(); | ||
assert_eq!(template.render(&mut context).unwrap(), | ||
Some("array: test2".to_owned())); | ||
} | ||
|
||
#[test] | ||
fn identifier_path_object_dot() { | ||
|
||
let options = LiquidOptions::with_known_blocks(); | ||
let template = "object_dot: {{ test_a[0].test_h }}\n"; | ||
|
||
let mut context = Context::new(); | ||
let mut internal = HashMap::new(); | ||
internal.insert("test_h".to_string(), Value::Num(5f32)); | ||
|
||
let test = Value::Array(vec![Value::Object(internal)]); | ||
context.set_val("test_a", test); | ||
|
||
let template = parse(template, options).unwrap(); | ||
assert_eq!(template.render(&mut context).unwrap(), | ||
Some("object_dot: 5\n".to_owned())); | ||
} | ||
|
||
#[test] | ||
fn identifier_path_object_string() { | ||
let options = LiquidOptions::with_known_blocks(); | ||
let template = "object_string: {{ test_a[0][\"test_h\"] }}\n"; | ||
|
||
let mut context = Context::new(); | ||
let mut internal = HashMap::new(); | ||
internal.insert("test_h".to_string(), Value::Num(5f32)); | ||
|
||
let test = Value::Array(vec![Value::Object(internal)]); | ||
context.set_val("test_a", test); | ||
|
||
let template = parse(template, options).unwrap(); | ||
assert_eq!(template.render(&mut context).unwrap(), | ||
Some("object_string: 5\n".to_owned())); | ||
} | ||
|
||
#[test] | ||
#[should_panic] | ||
fn identifier_path_subexpression() { | ||
let options = LiquidOptions::with_known_blocks(); | ||
let template = concat!("{% assign somevar=\"test_h\" %}", | ||
"result_string: {{ test_a[0][somevar] }}\n"); | ||
|
||
let mut context = Context::new(); | ||
let mut internal = HashMap::new(); | ||
internal.insert("test_h".to_string(), Value::Num(5f32)); | ||
|
||
let test = Value::Array(vec![Value::Object(internal)]); | ||
context.set_val("test_a", test); | ||
|
||
let template = parse(template, options).unwrap(); | ||
assert_eq!(template.render(&mut context).unwrap(), | ||
Some("object_string: 5\n".to_owned())); | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
counter
rather thanself.indexes.iter().enumerate()
?Context
to have a trait for the user-provided variables. Ideally, this would allow lazy lookups. To get maximize this with indexing, the trait would have some kind of lookup object (slice of Enum(string, index).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
counter
is an internal variable made to check the depth correctness. It's not used in template and is local to current function. I see no point to place it inside context.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, but I can't understand what you mean here. Could you explain a bit more please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context.get_val
implements some of the parsing algorithm for looking up a valueSo for
The current implementation requires copying all of
posts
before indexing into it.What'd be amazing is if we could do this
This would allow only the leaf to be copied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another use case for when copying will involve a lot of data: cobalt's
site.data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I got it. This is a complicated thing actualy. I think we could implement this in current model - where we are not using subexpressions.
But imagine some perfect case, when we could use subexpressions, like posts[ content[2*3] ]. In this case the subexpression should be rendered with the context as a parameter first. This is a main problem Rust is protecting us from: changing the context using the value from inside the context itself. I'm not sure if introducing a trait could solve it.
I see another kind of a solution here: read-only rendering, i.e. when Context doesn't change. Probably even a separate
RenderMut
should be introduced maybe to do non-readonly rendering where it's required explicitly and which I beleive is a more rare case than immutable one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel I'm missing something. I'm not seeing why
posts[ content[2*3] ]
is a problem, particularly that Rust is protecting us from and what a read-only Context gives us.My rough run through of how this would work:
For now, this just involves turning the
Identifier
path into&[PathPart]
and passing that toContext
.Long term, when/if we add more complex lookups, we should parse the inner expression first, render it, and make a
PathPart
out of it.This effectively transforms
into
(forgive my rough approximation of liquid syntax).
Note: these variables shouldn't actually need to be created in the Context, they should be able to live only in Rust.
and I don't see any problems with doing variable based lookups. When constructing a
PathPart
,IdentifierPath
would just do a lookup on the current value.