Add support for #define template functions

[whummer]

Following up from https://github.com/purcell/airspeed/issues/55 - add support for #define template functions. Addresses https://github.com/localstack/localstack/issues/5587

The implementation relies heavily on the existing #macro functionality, as @purcell suggested (thanks for laying out the solution, really helpful!).

Seems to cover the use case well, although there may still be a few things missing to achieve full compatibility with VLT (for use with AWS API Gateway). We can tackle those in a follow-up iteration, as needed.

If you have any suggestions on how to improve/optimize the implementation @purcell , please let us know! /cc @calvernaz

[calvernaz]

Hi @purcell, how are things? I'm wondering if you'll have time to look into this PR, this adds support to a feature we need to handle VTL templates when sometimes the user doesn't have much control over how to build them. I'll be here and @whummer to answer any questions, thanks!

[purcell]

Sorry, was travelling for business, so my service levels are degraded. Weekends are often when I try to catch up with requests.

I didn't mean to imply that the implementation of this would involve re-using the #macro machinery, rather that #macro would be a good replacement for #define in templates. (I understand that if users are supplying templates which use #define, that doesn't help much!)

I was starting to work on refactoring the code in this PR to remove a little of the hacky re-use of MacroDefinition, but I'm not sure this approach is the right one. The issue comes with namespaces: with the code in the PR, the body of a #define'd function will always use the namespace in which the function was defined, not the namespace where it's called. I expect that's not the Velocity behaviour, but perhaps you could clarify it? It's possible it doesn't make much difference in many cases, but we should try to make the behaviour correct.

I expected that any implementation of this would involve extending NameOrCall so that it could recognise a different type of function object (not simply a callable), and then call that with the current namespace.

[purcell]

An example failing test might be

    def test_define_with_local_namespace(self):
        template = airspeed.Template("#define ( $showindex )$foreach.index#end#foreach($x in [1,2,3])$showindex#end")
        self.assertEqual('012', template.merge({}))

Unsure what Velocity would do.

[whummer]

Thanks for looking into this @purcell !

Confirmed with latest Apache Velocity (version 2.3) that the output of the template above should be 012. 👍

It's possible it doesn't make much difference in many cases, but we should try to make the behaviour correct.

Agreed - would definitely be great to implement the correct behavior. Please let us know how to proceed - happy to dig more into it, if you can provide some guidance. Thanks so much for your help.

[purcell]

Okay, that was a bit painful, but I got this working in #57, and a new version has been released.