Start adding the built in filters

[bluntelk]

Hi There,

I have made a tentative first step in implementing the default twig filters. Is this format acceptable?

[tyler-sommer]

This looks fantastic, thanks for the contribution. I will take a better look tomorrow when I'm back on a computer, and we will get it merged.

Cheers!

[tyler-sommer]

Again, great work!

I really love the BuiltInFilters() pattern, and want to stick with it. However, I am concerned about the huge surface area having all of these filters exported in the "stick" package.

I propose we make another sub-package called "builtin". This way, we have a logical separation of the two packages: "stick" is the execution engine only, and "builtin" contains all core, builtin functionality. We could also mimic Twig and create a CoreExtension type in the "builtin" package that handles registering all builtins with any Env.

Of course, I will need to also move the AutoEscape functionality into this subpackage, which I have been debating for a while. But it seems clear with the number of builtin tests, filters, and the autoescaper functionality, we should put it all in its own package.

Let me know what you think-- I'm definitely open to suggestion. Thanks!

[bluntelk]

I think a builtin sub package would be awesome. we could then have a builtin.Filters() and builtin.Funcs() exported methods.

Not so sure I actually need to export the default filter functions... open to hiding them

[bluntelk]

Ahh, there's the snag - lots of structs and functions that are used for Filters are defined in the stick package.

You would need to move those to a "type" package to avoid an "import cycle not allowed" error

[tyler-sommer]

I've been pondering this issue for the last week (apologies for the silence, been out of town) and I'm not sure what to do.

I see a few options:

• Leave builtins in the stick package. Probably best to not export the individual filters, but that's kind of a problem in itself-- the filter functionality will still be an "exposed API" and so will need to be handled with the same care as the rest of the Go API in terms of BC breaks.
• Move Extensions, Filters, Funcs, and Tests (the type defs in stick.go) to a third package like api or the like. Then stick and builtin can reference the third package and work together without referencing each other directly.
• Move the builtins into a core extension, but don't bundle it with the stick package. This would mean that users would have to explicitly include the "builtin" functionality, but it does take care of the circular dep.
• Finally, and I'm not really sure on the feasibility, but we could use a database/sql package style of imports that just register user-defined stuff globally for all stick environments in the application. I haven't look into this much, but it may be an option.

I'm still considering what to do. Any input is very much appreciated!

[bluntelk]

May I suggest the K.I.S.S. principle here.

The simplest solution is to keep the default filters in the stick package. They are, after all, part of the twig spec.

By default do not export any of the default filters (or the default filters func), if users want to override them, they can write their own implementations (no need for extensibility).

This way the end user experience is the same as the twig package.

[tyler-sommer]

You're talking sense! Let's put the builtin stuff in the stick package and make the actual go definitions unexported. Thanks again! 👍

[bluntelk]

So I have gone ahead with the "Not exporting the default filters" option. easy enough change.

I added a couple more unit tests for the for loop batch filter thing that should pass when we sort out the parsing issues with it (yay unit tests).

[tyler-sommer]

I've exported a few more functions in the value API, and now have a working batch filter implementation. It's pretty messy, and I'm concerned about returning an invalid value, but here it is:

func FilterBatch(ctx stick.Context, val stick.Value, args ...stick.Value) stick.Value {
    if 2 != len(args) {
        // need 2 arguments
        return val
    }
    if !stick.IsIterable(val) {
        return val
    }
    blankValue := args[1]
    perSlice := int(stick.CoerceNumber(args[0]))
    if perSlice <= 1 {
        return val
    }
    l, _ := stick.Len(val)
    numSlices := int(math.Ceil(float64(l) / float64(perSlice)))
    out := make([][]stick.Value, numSlices)
    curr := make([]stick.Value, perSlice)
    i := 0
    j := 0
    _, err := stick.Iterate(val, func(k, v stick.Value, l stick.Loop) (bool, error) {
        curr[j] = v
        j++
        if j == perSlice {
            out[i] = curr
            curr = make([]stick.Value, perSlice)
            i++
            j = 0
        }
        return false, nil
    })
    if err != nil {
        // TODO: This is wack semantics
        return val
    }
    if j != perSlice {
        for h := j; j < perSlice; j++ {
            curr[h] = blankValue
        }
    }
    out[i] = curr
    return out
}

It's pretty bulky and awkward -- I'd like to improve that if possible -- but it uses the same semantics that stick itself uses when processing values, so that's a plus. Let me know what you think.

[tyler-sommer]

@bluntelk, I squashed these commits into 7de0833b7d7bd315a178baf34fc42edc9f0f6933 and merged into master, so I'm going to close this PR. I had hoped github would pick up the merge commit and consider this merged, but alas.

I also replaced your batch filter implementation with the semi-working one in my previous comment, and I'll likely rework the filter tests a bit, but I appreciate your help! Please feel free to continue contributing where you see fit :)

Again, thank you very much for all of your effort. Cheers! :+1: