Skip to content
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 6 commits into from
Nov 7, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/filters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,7 @@ pub fn map(input: &Value, args: &[Value]) -> FilterResult {
.filter_map(|v| {
v.as_object()
.map(|o| o.get(&property).cloned())
.map_or(None, |o| o)
.and_then(|o| o)
})
.collect();
Ok(Value::Array(result))
Expand Down
18 changes: 16 additions & 2 deletions src/lexer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ fn split_atom(block: &str) -> Vec<&str> {

lazy_static! {
static ref IDENTIFIER: Regex = Regex::new(r"[a-zA-Z_][\w-]*\??").unwrap();
static ref INDEX: Regex = Regex::new(r"^\.[a-zA-Z_][a-zA-Z0-9_-]*").unwrap();
static ref SINGLE_STRING_LITERAL: Regex = Regex::new(r"'[^']*'").unwrap();
static ref DOUBLE_STRING_LITERAL: Regex = Regex::new("\"[^\"]*\"").unwrap();
static ref NUMBER_LITERAL: Regex = Regex::new(r"^-?\d+(\.\d+)?$").unwrap();
Expand All @@ -108,7 +109,10 @@ lazy_static! {
pub fn granularize(block: &str) -> Result<Vec<Token>> {
let mut result = vec![];


let mut push_more;
for el in split_atom(block) {
push_more = None;
result.push(match &*el.trim() {
"" => continue,

Expand All @@ -134,8 +138,9 @@ pub fn granularize(block: &str) -> Result<Vec<Token>> {
"contains" => Comparison(Contains),
".." => DotDot,

x if SINGLE_STRING_LITERAL.is_match(x) ||
DOUBLE_STRING_LITERAL.is_match(x) => {
x
if SINGLE_STRING_LITERAL.is_match(x) ||
DOUBLE_STRING_LITERAL.is_match(x) => {
StringLiteral(x[1..x.len() - 1].to_owned())
}
x if NUMBER_LITERAL.is_match(x) => {
Expand All @@ -146,9 +151,18 @@ pub fn granularize(block: &str) -> Result<Vec<Token>> {
BooleanLiteral(x.parse::<bool>()
.expect(&format!("Could not parse {:?} as bool", x)))
}
x if INDEX.is_match(x) => {
let mut parts = x.splitn(2, '.');
parts.next().unwrap();
push_more = Some(vec![Identifier(parts.next().unwrap().to_owned())]);
Dot
}
x if IDENTIFIER.is_match(x) => Identifier(x.to_owned()),
x => return Err(Error::Lexer(format!("{} is not a valid identifier", x))),
});
if let Some(v) = push_more {
result.extend(v);
}
}

Ok(result)
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ mod filters;
mod value;
mod variable;
mod context;
mod path;

/// A trait for creating custom tags. This is a simple type alias for a function.
///
Expand Down
6 changes: 6 additions & 0 deletions src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use value::Value;
use variable::Variable;
use text::Text;
use output::{Output, FilterPrototype, Argument};
use path::IdentifierPath;
use token::Token;
use token::Token::*;
use lexer::Element::{self, Expression, Tag, Raw};
Expand Down Expand Up @@ -41,6 +42,11 @@ pub fn parse(elements: &[Element], options: &LiquidOptions) -> Result<Vec<Box<Re
// creates an expression, which wraps everything that gets rendered
fn parse_expression(tokens: &[Token], options: &LiquidOptions) -> Result<Box<Renderable>> {
match tokens[0] {
Identifier(ref x) if tokens.len() > 1 && (tokens[1] == Dot || tokens[1] == OpenSquare) => {
let mut result = IdentifierPath::new(x.clone());
try!(result.append_indexes(&tokens[1..]));
Ok(Box::new(result))
}
Identifier(ref x) if options.tags.contains_key(x) => {
options.tags[x](x, &tokens[1..], options)
}
Expand Down
140 changes: 140 additions & 0 deletions src/path.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
use Renderable;
use value::Value;
use context::Context;
use token::Token;
use token::Token::*;
use error::{Error, Result};

#[derive(Debug)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clone, Eq, etc?

pub struct IdentifierPath {
value: String,
indexes: Vec<Value>,
}

impl Renderable for IdentifierPath {
fn render(&self, context: &mut Context) -> Result<Option<String>> {
println!("{:?}", self);
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| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. There a reason you are using counter rather than self.indexes.iter().enumerate()?
  2. Long term I'm wanting to switch it so 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).

Copy link
Contributor Author

@Albibek Albibek Oct 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I was affraid to make iterator less readable because of more nesting.
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Sorry I wasn't clear. I was referring to the whole handling of value lookups and not counter. Both comments applied to the exact same line of code, making it harder.

Copy link
Contributor Author

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?

Copy link
Member

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 value

  1. Ideally we only implement the parsing algorithm in one place
  2. I want to copy the minimal part of a value as possible. Other clients wants to load data from a database (we'd change Context to have a trait to make this possible) and want to load the minimal amount from the database.

So for

{{ posts[5].custom.obj.arr[5] }}

The current implementation requires copying all of posts before indexing into it.

What'd be amazing is if we could do this

enum PathPart {
    ObjectIndex(&str),
    ArrayIndex(usize),
}

impl Context {
   fn get_val(&self, path: &[PathPart]) -> Option<Value> {
        ...
    }
}

This would allow only the leaf to be copied.

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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 to Context.

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

posts[ content[2*3] ]

into

{% assign _x = 2*3 %}
{% assign _y = content[_x] %}
posts[_y]

(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.

// go through error
let value = if let Ok(value) = value {
value
} else {
return value;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this just

let value = value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it's an early return from fold when value is error.
https://play.rust-lang.org/?gist=46038d66fc075362393e0eb30d9748dc&version=stable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped, sorry I missed that.

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 < 0f32 ||
x.round() > (::std::usize::MAX as f32) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does liquid support negative indexing like Python? Does it support slicing?

(and this code is one of the reasons why I'm considering native integer support)

Copy link
Contributor Author

@Albibek Albibek Oct 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither of docs I see say this explicitly, but a simple test in jekyll shows that negative indexing is allowed, ranges(I've tried [0..1] [0:1] [0-1]) are not

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this!

return Error::renderer("bad array index");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make the error include the index?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More thanks

}
let idx = x.round() as usize;
let value =
value
.get(idx)
.ok_or_else(|| Error::Render("index out of range".to_string()))?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you include the index and value's length?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yet again, thanks!

Ok(value)
}
(&Value::Array(_), _) => Error::renderer("bad array index type"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the error include a {:?} of value?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, thanks!

(&Value::Object(ref value), &Value::Str(ref x)) => {
let value =
value
.get(x)
.ok_or_else(|| Error::Render("object element not found".to_string()))?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you include x?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this!

Ok(value)
}
(&Value::Object(_), _) => Error::renderer("bad object index"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you {:?} the index?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this!

(value, _) if counter == 0 => Ok(value),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this case handling?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly other - leaf variants of Value I suppose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining.

I think I missed that this counts down. So you are doing special processing on the last element.

_ => Error::renderer("non-indexable element found while indexable expected"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you {:?} the element?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks! That'll make life much easier for people (lack of good error reporting is something we're slowly fixing).

}
});

match result {
Ok(result) => result.render(context),
Err(e) => Error::renderer(&format!("rendering error: {}", e)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use chain_err for this

result.chain_err(|| "Failed to render expression")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped, ? is much better choice, the chained error wouldn't have added much value.

}
}
}

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() {
let options = LiquidOptions::with_known_blocks();
let template = concat!("array: {{ test_a[0] }}\n",
"complex_dot: {{ test_a[0].test_h }}\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is test_a[0][var] a failure?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats the status of this?

"complex_string: {{ test_a[0][\"test_h\"] }}\n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make these separate tests?

Am I reading this right that the cases can be summarized as:

  • identifier_path_array_index
  • identifier_path_object_dot
  • identifier_path_object_index

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


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(concat!("array: test_h: 5\n",
"complex_dot: 5\n",
"complex_string: 5\n")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(random place)

Should we make it so all context.gets are IdentifierPath's?

The reason I'm considering this is we then have a central place of handling identifiers to ensure we are always consistent about how they are handled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dup of the first

.to_owned()));
}
}