-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add supported languages to materializations #5695
Conversation
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Just some minor questions regarding get_supported_languages
.
core/dbt/clients/jinja.py
Outdated
|
||
|
||
def get_supported_languages(node: jinja2.nodes.Macro) -> List[ModelLanguage]: | ||
no_args = not node.args or not node.defaults |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am assuming the length of args is always gonna be longer than defaults(contain both args and kwargs)? So node.defaults doesn't need to be checked here?
core/dbt/clients/jinja.py
Outdated
|
||
def get_supported_languages(node: jinja2.nodes.Macro) -> List[ModelLanguage]: | ||
no_args = not node.args or not node.defaults | ||
dif_args_and_defaults = len(node.args) != len(node.defaults) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be causing issue if we decided to add an arg to materialization Macro in the future. Since kwargs
are always going to be after args
, we can use the index to end to get things.
node.args = ["arg1", "supported_language", "another_key_for_kwarg"]
node.defaults = [['sql'], {"something_else": 1}]
you can get supported language by node.defaults[-(len(node.args) - lang_idx)]
…cro, update schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
* Add supported languages to materializations * Add changie entry * Linting * add more error and only get supported language for materialization macro, update schema * fix test and add more check Co-authored-by: Chenyu Li <chenyu.li@dbtlabs.com>
* Add supported languages to materializations * Add changie entry * Linting * add more error and only get supported language for materialization macro, update schema * fix test and add more check Co-authored-by: Chenyu Li <chenyu.li@dbtlabs.com>
* Add supported languages to materializations * Add changie entry * Linting * add more error and only get supported language for materialization macro, update schema * fix test and add more check Co-authored-by: Chenyu Li <chenyu.li@dbtlabs.com>
resolves #5569
Description
Adds support for materializations to have supported languages. If not set, this default to
sql
.Checklist
changie new
to create a changelog entry