Implement `contains` operator #157

badboy · 2018-01-09T17:28:25Z

This will panic if either side of the operator cannot be evaluated as a string.
It works for both variables and string literals as expected.

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 <-- Tons of unrelated changes.

epage · 2018-01-09T18:05:56Z

For reference, #155

epage

Thanks for getting this started!

I'm grateful for any contributions. You can keep owning this or hand off the rest of the work to me at any stage. I'd rather not burn you out :)

epage · 2018-01-09T18:10:33Z

src/tags/if_block.rs

+        None => return Error::renderer("Right-hand side of contains operator must be a string"),
+    };
+
+    Ok(a.contains(b))


contains supports two use cases

substring matches (page.title contains "Hello")

checking a hashmap if it contains a key (page contains "title")

This implements the first but for cobalt-org/cobalt.rs#363, it sounds like you need the second.

If you want, I can merge this now. Hopefully soon I can add support for checking for the object containing a key. Otherwise, feel free to add support for that in this PR or another one. Just let me know what you want.

epage · 2018-01-09T18:12:51Z

src/tags/if_block.rs

+fn contains_check(a: Value, b: Value) -> Result<bool> {
+    let a = match a.as_str() {
+        Some(s) => s,
+        None => return Error::renderer("Left-hand side of contains operator must be a string"),


(this is non-essential feedback, feel free to ignore)

Personally, I prefer

let a = a.as_str().ok_or("Left-hand side of contains operator must be a string")?;

or more like yours

let a = a.as_str().ok_or_else(|| Error::renderer("Left-hand side of contains operator must be a string"))?;

epage · 2018-01-09T18:14:10Z

src/tags/if_block.rs

+            .unwrap();
+
+        let mut context = Context::new();
+        let _output = template.render(&mut context).unwrap();


Instead of checking for panic, you could

let output = template.render(&mut context); assert!(output.is_err());

This makes maintenance a little easier because you don't have to worry about string matching the panic message (the error message can change without breaking the tests).

(also applies to the next test in case you decide to implement it)

epage · 2018-01-09T18:18:31Z

src/tags/if_block.rs

@@ -336,4 +350,75 @@ mod test {
        let output = template.render(&mut context).unwrap();
        assert_eq!(output, Some("fourth".to_owned()));
    }
+
+    #[test]
+    fn contains_comparison() {


Personally, I prefer the tests to be broken up by their purpose.

This is helpful for the person modifying tests and the person analyzing a test failure.

So for example,

#[test] fn contains_string_literals() { ... } #[test] fn contains_else() { ... } #[test] fn test contains_variable() { ... }

(I don't recommend literally copying my function names above when splitting this function because I'm making guesses as to your intent. You know what you intended with each test and this serves as a way for you to document it)

badboy · 2018-01-09T18:24:57Z

Thanks for quickly reviewing this. This was just a first shot at the minimal implementation.
Will follow your advice and split up tests and also implement the HashMap contains

badboy · 2018-01-09T19:11:12Z

Split up tests
Implemented for objects and arrays
Cleaned up extensive matches

epage · 2018-01-09T19:24:38Z

src/tags/if_block.rs

+    match *a {
+        Value::Str(ref val) => Ok(val.contains(b)),
+        Value::Object(ref obj) => Ok(obj.contains_key(b)),
+        Value::Array(ref arr) => {


I looked up contains but I thought it said it works for substrings and key checks.

Looking it up, instead its documented to work for substrings and array membership.
https://shopify.github.io/liquid/basics/operators/

But the implementation does work for objects's

https://github.com/Shopify/liquid/blob/master/lib/liquid/condition.rb#L18

https://ruby-doc.org/core-2.4.2/Hash.html#method-i-include-3F

So the only thing we're missing, compared to official liquid, is converting the right side to a string if the left side is a string.

That I'm perfectly happy postponing. I've started a major revamp for Value that will make it easier to implement all of these random liquid conversions / coercions that take place.

epage · 2018-01-09T19:27:20Z

This looks great! The work I see remaining to do

Resolve any CI failures
Squash commits
Please title the commit something like feat(if): Support contains operator
Please reference that this Fixes #155

Again, if there is any of that you aren't comfortable with or want to hand off, feel free to do so.

(Sorry we're using an odd version of rustfmt. I tried to mitigate that with writing out its config)

badboy · 2018-01-09T19:53:50Z

Ah no problem. With these detailed instructions it will be easy to do.

epage · 2018-01-09T19:57:01Z

Huh, I should actually give the "why" to things.

I use clog-cli to create changelists. By having feat: stuf in the title of the commit, I won't miss announcing your new feature in the changelog.

Fixes #155 will auto-close the issue and link it to the commit.

Squashing is to make the history cleaner which makes it easier to browse or git bisect.

badboy · 2018-01-09T20:22:32Z

I use clog-cli to create changelists. By having feat: stuf in the title of the commit, I won't miss announcing your new feature in the changelog.

Yup, I do something similar in other projects.
The rest is known as well. Thanks!

badboy · 2018-01-09T22:22:04Z

Oops, managed to hit the wrong button.

The contains operator works for strings (substring search), arrays (string in array) or objects (string key exists). The renderer will error if the right-hand side operator does not evaluate to a string, if the left-hand side is neither a string, array nor object or if the array contains non-string items. Fixes cobalt-org#155

badboy · 2018-01-09T22:58:42Z

The only remaining failure looks like a network error on traivs, restarting that test case might resolve it.

epage · 2018-01-09T23:19:18Z

Awesome! I kicked off the travis job.

I feel this will justify a liquid and cobalt release to get this into yours (and others) hands.

EDIT: Wrong CI

epage · 2018-01-09T23:46:39Z

btw the crate ecosystem seems to be trying to standardize the license. Please look at #7 and leave a comment if you accept the change.

epage · 2018-01-10T02:27:39Z

cargo publish on cobalt v0.11.1 is happening right now

badboy force-pushed the contains-ops branch from 3ee1bcc to 89d67de Compare January 9, 2018 17:32

epage requested changes Jan 9, 2018

View reviewed changes

epage reviewed Jan 9, 2018

View reviewed changes

epage approved these changes Jan 9, 2018

View reviewed changes

badboy force-pushed the contains-ops branch 4 times, most recently from 9703131 to 0172b32 Compare January 9, 2018 22:21

badboy closed this Jan 9, 2018

badboy reopened this Jan 9, 2018

badboy force-pushed the contains-ops branch from 0172b32 to a0d2720 Compare January 9, 2018 22:41

epage merged commit 14dea6a into cobalt-org:master Jan 9, 2018

epage mentioned this pull request Apr 10, 2018

Relicense under dual MIT/Apache-2.0 #7

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `contains` operator #157

Implement `contains` operator #157

badboy commented Jan 9, 2018 •

edited

Loading

epage commented Jan 9, 2018

epage left a comment

epage Jan 9, 2018

epage Jan 9, 2018

epage Jan 9, 2018

epage Jan 9, 2018

badboy commented Jan 9, 2018

badboy commented Jan 9, 2018

epage Jan 9, 2018

epage commented Jan 9, 2018

badboy commented Jan 9, 2018

epage commented Jan 9, 2018

badboy commented Jan 9, 2018

badboy commented Jan 9, 2018

badboy commented Jan 9, 2018

epage commented Jan 9, 2018 •

edited

Loading

epage commented Jan 9, 2018

epage commented Jan 10, 2018

Implement contains operator #157

Implement contains operator #157

Conversation

badboy commented Jan 9, 2018 • edited Loading

epage commented Jan 9, 2018

epage left a comment

Choose a reason for hiding this comment

epage Jan 9, 2018

Choose a reason for hiding this comment

epage Jan 9, 2018

Choose a reason for hiding this comment

epage Jan 9, 2018

Choose a reason for hiding this comment

epage Jan 9, 2018

Choose a reason for hiding this comment

badboy commented Jan 9, 2018

badboy commented Jan 9, 2018

epage Jan 9, 2018

Choose a reason for hiding this comment

epage commented Jan 9, 2018

badboy commented Jan 9, 2018

epage commented Jan 9, 2018

badboy commented Jan 9, 2018

badboy commented Jan 9, 2018

badboy commented Jan 9, 2018

epage commented Jan 9, 2018 • edited Loading

epage commented Jan 9, 2018

epage commented Jan 10, 2018

Implement `contains` operator #157

Implement `contains` operator #157

badboy commented Jan 9, 2018 •

edited

Loading

epage commented Jan 9, 2018 •

edited

Loading