Conversation
| builtins.update(jinja_globals) | ||
| builtin_globals.update( | ||
| { | ||
| **jinja_globals, |
There was a problem hiding this comment.
why are jinja_globals added to builtin and also returned return {**builtin_globals, **jinja_globals}?
There was a problem hiding this comment.
So that values in jinja_globals take precedence over the values that end up in the final builtin_globals.
There was a problem hiding this comment.
What I don't understand is if the methods are in builtin_globals, why do they need to be returned again?
- builtin_globals.update( { **jinja_globals,...} ) #builtin_globals now contains all jinja_globals
- return {**builtin_globals, **jinja_globals} # isn't this redundant since builtin_globals contains all jinja_globals?
There was a problem hiding this comment.
because var, ref, etc. are set after the builtin_globals.update( { **jinja_globals,...} ) and we want those keys from jinja_globals to take precedence. This is essentially a safe way to avoid doing if "<key>" not in jinja_globals:
| self._jinja_globals = create_builtins( | ||
| self._jinja_globals = create_builtin_globals( | ||
| jinja_macros=context.jinja_macros, | ||
| jinja_globals={ |
There was a problem hiding this comment.
is overrides a better term for this behavior?
There was a problem hiding this comment.
Not really, since this dict contains all globals and not just the ones that will be overridden.
There was a problem hiding this comment.
Can we either change the name of the kwarg to express how it behaves or add a method comment block that does so?
There was a problem hiding this comment.
The name of the kwargs is still accurate due to the discussion in the other thread.
3 participants
This PR adds support for the following DBT macros and filters: