support extra parameter 'condition' on 'expression_is_true' macro #147
Conversation
An optional parameter 'condition' can be passed to the 'expression_is_true' macro to assert the expression for all records which meet a condition. Closes #146
|
CI failed :( |
There was a problem hiding this comment.
One minor comment, but otherwise this looks very good to me. Going to kick off the test suite! Thanks for making this PR @dcereijodo :)
| @@ -1,12 +1,18 @@ | |||
| {% macro test_expression_is_true(model) %} | |||
|
|
|||
| {% set expression = kwargs.get('expression', kwargs.get('arg')) %} | |||
| {% set condition = kwargs.get('condition', 'true') %} | |||
There was a problem hiding this comment.
I think condition can be supplied as a proper keyword argument here, with the default set to true. We use this kwargs.get pattern to support both the model-level and column-level test specification syntax, but in the general case, I think it makes sense to keep arguments in the method signature where possible.
There was a problem hiding this comment.
I was actually a bit confused by the use of kwargs here, but I thought there should be a reason for it and kept it the same. I agree a normal argument is better. I'll send the changes with your suggestions. Thanks!
|
Cool, thanks for making that change! This will be good to merge when the tests pass. Side note: love your PR description.... we're going to add something like that as the default for PRs to this repo :) |
|
Thanks :) |
|
Merging this! Will cut a new release with this change on Monday :) 🎉 |
An optional parameter 'condition' can be passed to the
'expression_is_true' macro to assert the expression for all records
which meet a condition. Closes #146
Checklist