Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rework macro import #332

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jjkavalam
Copy link

Problem

The issue this PR fixes is #330

Background: How a macro is currently executed ?

  1. importNode.Execute / macroNode.Execute places the macroNode into current context (Private)
  2. variable finds macroNode in the current context (Private)
  3. macroNode.call calls the wrapper with context of the caller

Since, macro definitions are looked up from the execution context,
only those macros that are available there can be successfully lookedup when a variable evaluates.

The variable should also be able to look into the lexical context (i.e. the containing document)
and find macroNodes there as well.

{# filename: macros.j2 #}

{% macro child() %}Child{% endmacro %}

{% macro main() export %}
Main calls {{ child() }}                            // 3. looks up "child" in ctx ? does not exist !
{% endmacro %}
---
{# filename: test1.j2 #}

{% import "macros.j2" main %}                       // 1. ctx["main"] = macroNode#main

{{ main() }}                                        // 2. lookup "main" in context

Maintain lexicalCtx as well; and it should be attached to the file.
Variable resolution should lookup there as well.

Whenever a macro definition or import is executed, it should place the definitions in the lexical scope instead.

This solution at a glance

Following TypeScript code snippet shows the proposed mechanism for dealing with macros that can solves the issue mentioned above.

class MacroNode {
    name: string;
    call(ctx: ExecutionContext) {

    }
    execute() {
        // don't do anything; macros are no more looked up from the Private context
    }
    static parse(template: Template): MacroNode {
        const node = new MacroNode();

        // register macro node on the template
        template.ownMacroNodes.push(node);

        return node;
    }
}

class ImportNode {
    macros: MacroNode[];
    template: Template;
    execute() {
        // don't do anything; macros are no more looked up from the Private context
    }
    static parse(template: Template): ImportNode {
        const node = new ImportNode();

        // register import nodes on the template
        node.template = template;
        template.macroImportNodes.push(node);

        return node;
    }
}

class Template {
    ownMacroNodes: MacroNode[];
    macroImportNodes: ImportNode[];
}

class ExecutionContext {
    template: Template;
}

class VariableResolver {
    resolve(ctx: ExecutionContext, varname: string) {
        let macroNode = find(ctx.template.ownMacroNodes, varname);
        if (macroNode) {
            // call own macros with same context
            macroNode.call(ctx);
        }
        else {
            for (const importNode of ctx.template.macroImportNodes) {
                macroNode = find(importNode.macros, varname);
                if (macroNode) {
                    const thisTemplate = ctx.template;
                    ctx.template = importNode.template;
                    // call imported macros with adjusted context
                    macroNode.call(ctx);
                    ctx.template = thisTemplate;
                    break;
                }
            }
        }
    }
}

function find(macros: MacroNode[], name: string): MacroNode {
    throw new Error("Function not implemented.");
}

@sonarcloud
Copy link

sonarcloud bot commented Feb 12, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@jjkavalam jjkavalam mentioned this pull request Feb 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant