Fix included macros aren't working

[gebolze]

Fix for #551

Macros included using the include statement are always undefined because they are defined within the scope of the include statement. To prevent this, this pull-request changes the implementation of the macro statement to always register the macro to the root scope.

[sebastienros]

I was thinking about how this could be an issue when multiple files are included, or when a macro would then overwrite and existing property with the same name (from another template). Or you think you are in an isolated template but your local macro will replace the RootScope property with the same name.

I checked Jinja (where I think I took the idea from, and why this feature is behind a flag as it's not standard). https://jinja.palletsprojects.com/en/2.10.x/templates/#import
What they do is make it explicit, so if you want to import the macros from a template you just say it, and pass a variable name that will be in your current scope.

Do you think this could be a better solution? Local macros don't have to be imported so there is no breaking change. It would only require a new tag, copied from include and render. If so keep this PR around in case the other way is too hard to implement.

[gebolze]

I've updated the PR. I've changed the implementation and introduced a new from tag that allows importing macros from external templates.

{% from 'file' %} imports all defined macros.
{% from 'file' import foo, bar %} imports only foo, bar.

I've implemented the TagStatement such that it doesn't render anything to the output and only import macros to the local scope. This is a bit opinionated, but I think the from tag should not be used to for scenarios where include or render could be used.

[sebastienros]

I was not expecting anybody to be able to do this without asking more questions or saying "it's too complicated", good job!

Give me some time to look again into Jinja and if their way for naming macros has advantages. I don't think it would be more complex than what you implemented, just checking the user cases and how simple the usage is.

[sebastienros]

The PR is not changing the behavior wrt includes, so this will still work, right? Not saying this should be removed. I also remember that include is not recommended in Liquid (because like in this case it leaks to the caller).

[gebolze]

The PR is not changing the behavior wrt includes, so this will still work, right?
Yes behavior of includes isn't changed by this PR. But I would still say that include can't be used to add external macros to the local scope. The following example won't work:

functions.liquid

{%- macro hello_world() -%}
Hello world!
[%- endmacro -%}

template.liquid

{%- include 'functions' %}
{{ hello_world() }}

This will not render Hello world!

Actually the opposite will work.
You can access a marco declared in a template from the included file.

Hope this answers your question.

[sebastienros]

This is a bit opinionated, but I think the from tag should not be used to for scenarios where include or render could be used.

Are you talking about not rendering the imported template? Is so then yes definitely should not render, just import functions from it.

[sebastienros]

Done with checking Jinja again. Can you confirm that what you did is compliant with Jinja's syntax, but you just don't handle the as keywork either in the from or the import expressions? But this could be added later.

Examples:

{% from 'forms.html' import input as input_field, textarea %} -> imports input but names it input_field locally, then imports textarea

{% import 'forms.html' as forms %} -> imports all function but as forms.input and forms.textarea.