New filters #74

djwf · 2017-02-18T05:11:54Z

I'm putting all the filters I create from the list in issue #11 here, as they are part of the standard, and the filter in pull request #73 is not.

Let me know if I should merge these two or split all filters into their own pull requests. Also, note that I'll use this to add more filters, and will leave a note when I've finished adding filters. If you'd prefer I not work that way, let me know and I'll change it up.

djwf · 2017-02-18T05:12:52Z

Whoops! My apologies: I need to rebase this off of master, rather than off of my other pull request. I'll do that after I get some sleep.

djwf · 2017-02-18T16:25:33Z

@johannhof: I am confused as to which tests would go in the src/filters.rs file, and which would go in tests/filters.rs. Can you give me some guidance on that?

djwf · 2017-02-20T20:37:32Z

@johannhof: I'm working on truncate, and think liquid-rust would be well served by using proper unicode parsing of strings, thus grapheme clusters rather than characters. I have added a dependency to an external crate for unicode segmentation for that purpose. Please let me know if I should back that change out.

johannhof · 2017-02-21T14:58:51Z

@djwf Thanks so much for your work on this. Just wanted to let you know it's not been forgotten, I'm simply traveling between continents for work. I'll get to reviewing this today or tomorrow. :)

djwf · 2017-02-21T19:58:46Z

@johannhof No worries at all: I'll just keep plugging away at them. Note that I'm using the list of filters at the liquid markup web site as the authoritative list, as I noticed some of those missing from your list in #11 .

johannhof · 2017-02-25T16:04:30Z

Ok, looking at this now. The re-ordering makes it a bit hard to see what exactly changed, but I'll manage. As to your questions:

src/filters.rs is probably the best place to put tests. tests/filters.rs is intended to test cases where you want the code to go through the whole lexer, parser, etc. before running the filter (although it includes "normal" tests because I was sloppy with review)
grapheme clusters: without having read the code yet, I agree that this is a good idea in general. I'm not sure how the Ruby version does it (as we'd ideally want to produce the same output at the end of they day), but I guess that shouldn't stop us from doing it the right way. I wonder if there are places in the lexer that could benefit from this, too. But that's irrelevant to this already pretty big PR.

Cheers

johannhof

Pretty good. Just a couple of nits. Thanks so much!

Note that I did not very thoroughly look at the reordered functions (GitHubs diff sucks for that). I'll just assume you didn't plant a trojan horse here.

johannhof · 2017-02-25T20:30:16Z

src/filters.rs

+pub fn replace_first(input: &Value, args: &[Value]) -> FilterResult {
+    if args.len() != 2 {
+        return Err(InvalidArgumentCount(format!("expected 2, {} given", args.len())));
+    }


While you're at it, you can replace this with check_args_len

Got it, done.

johannhof · 2017-02-25T21:33:47Z

src/filters.rs

@@ -670,6 +670,23 @@ pub fn truncatewords(input: &Value, args: &[Value]) -> FilterResult {
    }
 }

+/// Removes any duplicate elements in an array.


Maybe leave a comment that this operates in O(n^2) worst-case, which should be fine IMO.

Got it, done.

johannhof · 2017-02-25T22:04:01Z

src/template.rs

@@ -1,10 +1,44 @@
 use Renderable;
 use context::Context;
-use filters::{size, upcase, downcase, capitalize, minus, plus, times, divided_by, ceil, floor,
-              round, prepend, append, first, last, pluralize, replace_first, replace, date, sort,
-              slice, modulo, escape, escape_once, remove_first, remove, strip_html, truncatewords};


Was this change made because of rustfmt? I liked the grouped version a bit better.

That was just because it makes it match the list of filters in the impl Renderable ... section, and it made it easier to see where/what was added. I don't have strong feelings about it either way, so I'll put it back to a grouped version (ordered alphabetically).

johannhof · 2017-02-26T21:49:27Z

src/filters.rs

+    try!(check_args_len(args, 0));
+    match *input {
+        Value::Array(ref array) => {
+            let mut reversed = array.clone();


Note to self: Look if we can reduce allocation in filters (by modifying output.rs) before 1.0

johannhof · 2017-02-26T21:55:35Z

src/filters.rs

+    let ellipsis = "...".to_string();
+    let append = match args.get(1) {
+        Some(&Str(ref x)) => x,
+        _ => &ellipsis,


You can replace this with just "..."

Ah damn I just noticed you took that from truncatewords. Can you please fix this up in truncatewords, too? Thank you!

Yep: fixed both truncate and truncatewords.

djwf · 2017-02-28T04:00:52Z

Thanks for the answers and the review: note that the most recent nightly of clippy is complaining about the five uses of lazy_static! (four in lexer.rs, one in filters.rs) so (on my end, at least) the rustup run nightly cargo clippy fails.

Also, my rustfmt (0.7.1 ()) formats the grouped filter use statement as follows:

use filters::{abs, append, capitalize, ceil, date, divided_by, downcase, escape, escape_once, first,
              floor, join, last, lstrip, minus, modulo, newline_to_br, pluralize, plus, prepend,
              remove, remove_first, replace, replace_first, reverse, round, rstrip, size, slice,
              sort, split, strip, strip_html, strip_newlines, times, truncate, truncatewords, uniq,
              upcase};

The rustfmt for the Travis CI builds, however, formats the grouped filter use statement differently:

use filters::{abs, append, capitalize, ceil, date, divided_by, downcase, escape, escape_once,
              first, floor, join, last, lstrip, minus, modulo, newline_to_br, pluralize, plus,
              prepend, remove, remove_first, replace, replace_first, reverse, round, rstrip, size,
              slice, sort, split, strip, strip_html, strip_newlines, times, truncate,
              truncatewords, uniq, upcase};

No big deal, but I figured you'd want to know.

johannhof · 2017-02-28T10:29:05Z

note that the most recent nightly of clippy is complaining about the five uses of lazy_static! (four in lexer.rs, one in filters.rs) so (on my end, at least) the rustup run nightly cargo clippy fails.

Seems like there's an issue around that recently added rule: https://github.com/Manishearth/rust-clippy/issues/1580

You can ignore it by adding #![cfg_attr(feature="cargo-clippy", allow(zero_ptr))] to lib.rs.

The rustfmt for the Travis CI builds, however, formats the grouped filter use statement differently:

Feel free to update the version to v0.7.1-20170120 here: https://github.com/cobalt-org/liquid-rust/blob/master/.travis.yml#L10

johannhof · 2017-02-28T10:34:28Z

You can ignore it by adding #![cfg_attr(feature="cargo-clippy", allow(zero_ptr))] to lib.rs.

Nevermind, I just merged #80 so you'll just have to rebase :)

- Split helper functions into separate section. - Sort filter functions to simplify finding a navigation. - Sort unit tests to maintain the same ordering as the filters.

- Introduces dependency on unicode_segmentation crate - Uses segmentation of strings based on grapheme clusters to properly handle unicode strings

This reverts commit 10f5e84. Apparently the new rustfmt requires GLIBC_2.18, to fix later.

johannhof · 2017-03-04T12:55:33Z

Looks great!

johannhof mentioned this pull request Feb 18, 2017

Add date_in_tz filter. #73

Merged

djwf force-pushed the new-filters branch from 1bc0396 to c193f53 Compare February 18, 2017 15:41

fiji-flo mentioned this pull request Feb 25, 2017

add default filter #76

Merged

johannhof approved these changes Feb 26, 2017

View reviewed changes

djwf added 17 commits February 28, 2017 06:40

Initial implementation of the reverse filter

286bffa

Initial implementation of the newline_to_br filter

a23e953

Split filters into separate statements and sort

3186ae0

Sort filters/tests

8473009

- Split helper functions into separate section. - Sort filter functions to simplify finding a navigation. - Sort unit tests to maintain the same ordering as the filters.

Initial implementation of the abs filter

ea1a3a2

Convert doc strings to comments

6619063

Format via rustfmt

353b309

Use short enum variant names

d3a485f

Initial implementation of the lstrip filter

b898a9f

Initial implementation of the rstrip filter

53f4788

Remove unneeded return statements

96bf56d

Initial implementation of the strip filter

495eaeb

Fix docstring over 100 characters

7ca8280

Move {,l,r}strip unit tests into proper order

a3e6b4a

Add two new, adjust some old, tests for abs

0e1409f

Initial implementation of the strip_newlines filter

b0096b9

Initial implementation of the truncate filter

0ce4a03

- Introduces dependency on unicode_segmentation crate - Uses segmentation of strings based on grapheme clusters to properly handle unicode strings

djwf added 10 commits February 28, 2017 06:40

Initial implementation of the uniq filter

fd78c6a

Use standard function to check args length

f21b223

Remove allocation

3bba05c

Add lines for readability/parallelism

184d45a

Note complexity of dedupe

22ea4bf

Note source of externally sourced tests

fd5b574

Group filter use statements

0a12e65

Remove unimplemented filter

395bcb7

Fix wrapping since rustfmt won't.

360bed9

Update rustfmt version used by Travis

10f5e84

djwf force-pushed the new-filters branch from 7007d9b to 10f5e84 Compare February 28, 2017 11:44

Revert "Update rustfmt version used by Travis"

724397b

This reverts commit 10f5e84. Apparently the new rustfmt requires GLIBC_2.18, to fix later.

johannhof merged commit 323ecd6 into cobalt-org:master Mar 4, 2017

djwf deleted the new-filters branch March 19, 2017 01:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New filters #74

New filters #74

djwf commented Feb 18, 2017 •

edited

Loading

djwf commented Feb 18, 2017

djwf commented Feb 18, 2017

djwf commented Feb 20, 2017 •

edited

Loading

johannhof commented Feb 21, 2017

djwf commented Feb 21, 2017

johannhof commented Feb 25, 2017

johannhof left a comment

johannhof Feb 25, 2017

djwf Feb 28, 2017

johannhof Feb 25, 2017

djwf Feb 28, 2017

johannhof Feb 25, 2017

djwf Feb 28, 2017

johannhof Feb 26, 2017

johannhof Feb 26, 2017

johannhof Feb 26, 2017

djwf Feb 28, 2017

djwf commented Feb 28, 2017

johannhof commented Feb 28, 2017

johannhof commented Feb 28, 2017

johannhof commented Mar 4, 2017

New filters #74

New filters #74

Conversation

djwf commented Feb 18, 2017 • edited Loading

djwf commented Feb 18, 2017

djwf commented Feb 18, 2017

djwf commented Feb 20, 2017 • edited Loading

johannhof commented Feb 21, 2017

djwf commented Feb 21, 2017

johannhof commented Feb 25, 2017

johannhof 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

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

djwf commented Feb 28, 2017

johannhof commented Feb 28, 2017

johannhof commented Feb 28, 2017

johannhof commented Mar 4, 2017

djwf commented Feb 18, 2017 •

edited

Loading

djwf commented Feb 20, 2017 •

edited

Loading