-
Notifications
You must be signed in to change notification settings - Fork 4
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 expression_is_false
dbt test macro
#464
Add expression_is_false
dbt test macro
#464
Conversation
This is ready to go, though these test names feel backwards to me. Probably not important. |
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.
Nice! Real quick before we merge this in, could we add some simple docs for expression_is_false
to the generic tests README?
{% test expression_is_false( | ||
model, column_name, expression, additional_select_columns=[] | ||
) %} | ||
{%- set select_columns_csv = format_additional_select_columns( | ||
additional_select_columns | ||
) -%} | ||
{%- if column_name -%} | ||
{%- set columns_csv = column_name -%} | ||
{%- if select_columns_csv -%} | ||
{%- set columns_csv = columns_csv ~ ", " ~ select_columns_csv -%} | ||
{%- endif -%} | ||
{%- elif select_columns_csv -%} {%- set columns_csv = select_columns_csv -%} | ||
{%- else -%} {%- set columns_csv = "1 AS fail" -%} | ||
{%- endif -%} | ||
|
||
select {{ columns_csv }} | ||
from {{ model }} | ||
where ({{ column_name }} {{ expression }}) | ||
{% endtest %} |
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.
[Thought, non-blocking] Looks like everything here is identical to expression_is_true
except for the lack of a not
operator on line 26. Perhaps a sign that we could factor out the shared logic into a third macro that expression_is_true
and expression_is_false
both depend on? Not required by any means, but something to think about!
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'm not opposed to that at all.
Our current
expression_is_true
dbt tests can lead to some weird backwards engineering trying to figure out how to invert logic given how it employsNOT
before theWHERE
clause it tests. Having an alternative to avoid these situations would be helpful.