Add indexed access to context values #141

Albibek · 2017-10-27T12:12:12Z

This fixes #127 adding feature to access identifiers via index

array indexes arr[0] are supported
object indexes obj["child"] are supported and interchangeable with obj.child
index expressions are not yet supported.

Checklist

Tests created for any new feature or regression tests for bugfixes.
cargo test succeeds
rustup run nightly cargo clippy succeeds
cargo fmt -- --write-mode=diff succeeds

epage

Thanks for doing this. I really appreciate it (been putting off having to figure out the parser.

Feel welcome to have a conversation about "not now" on the comments I made.

epage · 2017-10-27T12:58:40Z

src/path.rs

+                value
+            } else {
+                return value;
+            };


Isn't this just

let value = value?

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

Dropped, sorry I missed that.

epage · 2017-10-27T13:00:44Z

src/path.rs

+                    // because zero is not counted normal
+                    if (*x != 0f32 && !x.is_normal()) || *x < 0f32 ||
+                       x.round() > (::std::usize::MAX as f32) {
+                        return Error::renderer("bad array index");


Could you make the error include the index?

More thanks

epage · 2017-10-27T13:01:16Z

src/path.rs

+                    // 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) {


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)

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

Thanks for looking into this!

epage · 2017-10-27T13:01:52Z

src/path.rs

+                    let value =
+                        value
+                            .get(idx)
+                            .ok_or_else(|| Error::Render("index out of range".to_string()))?;


Can you include the index and value's length?

Yet again, thanks!

epage · 2017-10-27T13:02:31Z

src/path.rs

+                            .ok_or_else(|| Error::Render("index out of range".to_string()))?;
+                    Ok(value)
+                }
+                (&Value::Array(_), _) => Error::renderer("bad array index type"),


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

Again, thanks!

epage · 2017-10-27T13:06:01Z

src/path.rs

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


You can use chain_err for this

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

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

epage · 2017-10-27T13:06:41Z

src/path.rs

+            .clone();
+
+        let mut counter = self.indexes.len();
+        let result = self.indexes.iter().fold(Ok(&value), |value, index| {


There a reason you are using counter rather than self.indexes.iter().enumerate()?

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

I was affraid to make iterator less readable because of more nesting.

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.

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.

I'm sorry, but I can't understand what you mean here. Could you explain a bit more please?

Context.get_val implements some of the parsing algorithm for looking up a value

Ideally we only implement the parsing algorithm in one place

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.

Another use case for when copying will involve a lot of data: cobalt's site.data

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.

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.

epage · 2017-10-27T13:12:04Z

src/path.rs

+        let options = LiquidOptions::with_known_blocks();
+        let template = concat!("array: {{ test_a[0] }}\n",
+                               "complex_dot: {{ test_a[0].test_h }}\n",
+                               "complex_string: {{ test_a[0][\"test_h\"] }}\n");


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

epage · 2017-10-27T13:17:04Z

src/path.rs

+        assert_eq!(template.render(&mut context).unwrap(),
+                   Some(concat!("array: test_h: 5\n",
+                                "complex_dot: 5\n",
+                                "complex_string: 5\n")


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

Dup of the first

epage · 2017-10-27T13:17:41Z

src/path.rs

+    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",


is test_a[0][var] a failure?

Whats the status of this?

epage · 2017-10-30T23:03:42Z

src/path.rs

+use token::Token::*;
+use error::{Error, Result};
+
+#[derive(Debug)]


Clone, Eq, etc?

epage · 2017-10-30T23:06:39Z

src/path.rs

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


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

epage · 2017-10-30T23:07:46Z

src/path.rs

+                (value, _) if counter == 0 => Ok(value),
+                (value, _) => {
+                    Error::renderer(
+                        &format!("expected indexable element, but founr '{:?}'", value)


founr -> found :)

epage · 2017-10-30T23:08:53Z

src/path.rs

+                    Ok(value)
+                }
+                (&Value::Object(_), _) => Error::renderer("bad object index"),
+                (value, _) if counter == 0 => Ok(value),


Thanks for explaining.

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

epage · 2017-10-30T23:23:46Z

So looking back, it sounds like the outstanding items are:

Fix travis
Fix typo while at it :)
Respond to is test_a[0][var] a failure?
(not blocking submission) Consolidate regular path parsing and indexed path parsing
(not blocking submission) Switch to constructing a &[PathPart] and passing that to Context.

Albibek · 2017-11-02T07:22:12Z

My current fresh rustfmt (0.9.0) refuses to format the lexer.rs as travis requires. The warning will probably will go away as Tavis updates rustfmt. I can fix it to a custom formatting if you want, but it'll be a real pain to maintain this file with newer rustfmt installed.

I've also added the should_panic test case to use when subexpressions arrive.

I'm sorry, but I currently don't have too much time to make subexpression support and I really think it should be done after grammar-based parser is implemented.

My main arguments for deferring it are (I may be wrong here, feel free to discuss it):

I'd like to avoid writing code doing same things twice since expression based parser would make lots of code to be rewritten
I'd like to avoid increasing amount of custom token parsing code to force ourselves for parser to happen earlier

epage · 2017-11-02T13:49:34Z

My current fresh rustfmt (0.9.0) refuses to format the lexer.rs as travis requires. The warning will probably will go away as Tavis updates rustfmt. I can fix it to a custom formatting if you want, but it'll be a real pain to maintain this file with newer rustfmt installed.

Huh, for some reason I( thought 0.9.0 used nightly before they decided to switch it over to rustfmt-nightly.

Would you prefer to comply with 0.8.6 first and I do the upgrade afterwords or for me to do the upgrade now, possibly causing some rebase pain for you?

I'm sorry, but I currently don't have too much time to make subexpression support and I really think it should be done after grammar-based parser is implemented.

I'm sorry if I gave you the impression that you should! I know it'd be complex with the current parser and I'm grateful for what you have done!

I don't mean questions to be a passive aggressive way of saying "do this" but as a means to make sure implicit assumptions become explicit so people know why you did what you did and what limitations might exist.

Albibek · 2017-11-03T11:59:35Z

Preformed the formatting with 0.8.6.

And of course I didn't in any way imply that anyone is forcing me to do something and I'm totally no offence about this whole PR. The apology above is more about my personal feeling about work not done as it could be done if I had more time.

epage · 2017-11-03T14:46:24Z

And of course I didn't in any way imply that anyone is forcing me to do something and I'm totally no offence about this whole PR.

Glad to hear. I just want to make sure I've been communicating clearly to avoid burning people out and driving away contributors.

epage · 2017-11-03T14:47:34Z

Thank you for leaving each commit for me to see the differences between each round of feedback.

At this point, could you clean up the commit history and post here when done?
(force pushes don't always cause email notifications)?

Thanks for getting this done!

Albibek · 2017-11-03T15:01:55Z

Do you mean squashing commits?

epage · 2017-11-03T15:22:43Z

Do you mean squashing commits?

The exact format I leave to you; you know your changes best.

My recommendation

Each commit should stand on its own
Each commit should build cleanly (don't stress trying to test each one)

So for example, when a commit is made to satisfy the CI, it could probably be squashed. I can go either way on improving error messages and negative indexing being separate commits or squashed, as long as the commit messages are descriptive enough.

Exceptions are gladly granted if trying to re-arrange commit history becomes messy due to conflicts.

epage · 2017-11-07T05:25:31Z

Went ahead and did a squash+merge because I'm going to do a release soon.

* **syntax:** Add `arr[0]` and `obj["name"]` indexing (PR #141, fixes #127) * **value:** Add nil value to support foreign data (PR #140)

Sergey Noskov added 2 commits October 27, 2017 14:49

Add indexed access to context values

43fddab

Fix lints and formatting

8c51d00

epage requested changes Oct 27, 2017

View reviewed changes

Sergey Noskov added 2 commits October 27, 2017 18:11

More informative error messages

58e99fe

Add negative index, fix formatting

79debf7

epage reviewed Oct 30, 2017

View reviewed changes

Subexpression test, small fixes

fa86130

Fix formatting to comply rustfmt 0.8.6

3aa5feb

epage approved these changes Nov 3, 2017

View reviewed changes

epage merged commit da06c73 into cobalt-org:master Nov 7, 2017

epage mentioned this pull request Nov 8, 2017

Allow expressions in indexed variables #145

Closed

epage added a commit that referenced this pull request Nov 8, 2017

chore: Release 0.11

742896f

* **syntax:** Add `arr[0]` and `obj["name"]` indexing (PR #141, fixes #127) * **value:** Add nil value to support foreign data (PR #140)

epage mentioned this pull request Apr 10, 2018

Relicense under dual MIT/Apache-2.0 #7

Open

4 tasks

Add indexed access to context values #141

Add indexed access to context values #141

Conversation

Albibek commented Oct 27, 2017 • edited by epage Loading

epage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Albibek Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Albibek Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage commented Oct 30, 2017

Albibek commented Nov 2, 2017

epage commented Nov 2, 2017

Albibek commented Nov 3, 2017

epage commented Nov 3, 2017

epage commented Nov 3, 2017

Albibek commented Nov 3, 2017

epage commented Nov 3, 2017

epage commented Nov 7, 2017

Albibek commented Oct 27, 2017 •

edited by epage

Loading

Albibek Oct 27, 2017 •

edited

Loading

Albibek Oct 27, 2017 •

edited

Loading