Skip to content

added exportmacro identifier in for loop #3

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

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

Conversation

sprksh
Copy link
Collaborator

@sprksh sprksh commented Sep 21, 2022

Description of the change

Adding functionality to be able to define and import macro within a for loop to be used in the rest of the template. Key is using exportmacros as an identifier in for loop. In the below template abc = [1,2]

{% for x in abc exportmacros %}
    {% if x == 1 %}
        {% import "x.template" m4 %}
    {% else %}
        {% macro m5() export %}
            m5 value
        {% endmacro %}
    {% endif %}
{% endfor %}
--
{{m5()}}
{{m4()}}

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related issues

Fix #1

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers mentioned in a comment
  • Changes have been reviewed by at least one other engineer
  • Issue from task tracker has a link to this pull request

@sprksh sprksh changed the title added exportmacro identifier in for loop, tests remaining added exportmacro identifier in for loop Sep 21, 2022
Copy link
Collaborator

@nishantsharma nishantsharma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jdreamerz
Copy link

This seems like it will be very easy to overwrite macros and difficult to define multiple ones. Why do we want this? Seems like you could just define a macro you want and pass in the item you're looping over and it's key and get the same functionality. Without dynamic macro names, I don't really see the point?

@nishantsharma
Copy link
Collaborator

This seems like it will be very easy to overwrite macros and difficult to define multiple ones. Why do we want this? Seems like you could just define a macro you want and pass in the item you're looping over and it's key and get the same functionality. Without dynamic macro names, I don't really see the point?

We want to loop over feature YAML contents and define and possibly reuse macros contained there.

@jdreamerz
Copy link

You added the exec keyword already, wouldn't you just loop over the user defined macros and create a macro template based on them, then add it to the template as exec userMacros or something?

@jdreamerz
Copy link

jdreamerz commented Sep 22, 2022

To define macros contained in the feature yaml using something like this you would need the macro name to be dynamic based on the name the user gave to it. So, you would need to set the macro name based on a variable. Could you do that?

@sprksh
Copy link
Collaborator Author

sprksh commented Sep 22, 2022

The issue with forloop is that pongo creates a child execution context and the macros defined in for loop stay in the scope of that forloop only.. in order to be able to use that macro outside the forloop we have to export the macro to parent context. That is what is targeted in this PR.

Dynamic naming of macro is not possible. can we use string formatting to define the macro name prior to parsing it by pongo? that will fulfil the requirement of dynamic naming i suppose.

@jdreamerz
Copy link

jdreamerz commented Sep 22, 2022

The issue with forloop is that pongo creates a child execution context and the macros defined in for loop stay in the scope of that forloop only.. in order to be able to use that macro outside the forloop we have to export the macro to parent context. That is what is targeted in this PR.

Dynamic naming of macro is not possible. can we use string formatting to define the macro name prior to parsing it by pongo? that will fulfil the requirement of dynamic naming i suppose.

That is why it seems like this won't support the idea of defining the macros defined in a feature table in pongo. I think you would need to create a userMacros template or something by using pongo to create a pongo template, then import it using exec. But you don't need this to do that. This seems like basically adding a keyword to a loop saying every closure function defined inside it should be global, which strikes me as a bad idea.

@jdreamerz
Copy link

If you do need to do this for some reason I'm not seeing, could you make it so that by default all macros could be exported from a loop, but to do so need the global keyword on them? That would match more closely to a programming language expectation. Note that Jinja - which is very similar to pongo2 and is the templating language dbt uses - also scopes loops (and other blocks) and does not allow exporting variables inside them to outside their scope:

https://jinja.palletsprojects.com/en/3.0.x/templates/ - look at Scoping Behavior

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.

3 participants