Fix boolean logic in expect_compound_columns_to_be_unique ignore_row_if
#202
Conversation
There was a problem hiding this comment.
I believe this will fix the bug. I think a couple tweaks could make it nicer but no complaint from me if it shipped like this, I'm also happy to PR those myself in a follow up.
I also had the thought, is it possible that this logic shows up in other expectations? I didn't investigate this at all.
| {% endfor %} | ||
| ) | ||
| {%- endif -%} | ||
| {%- set op = "and" if ignore_row_if == "all_values_are_missing" else "or" -%} |
There was a problem hiding this comment.
This looks correct now, but closes off the "third behavior" of "never ignore" and somewhat obfuscates what behavior to expect if I pass a value other than "all_values_are_missing".
Did you mean to eliminate the (heretofore undocumented) third behavior?
What would you think of adding a guard prior to this that ensures the value of ignore_row_if is an expected one? Even if it's not documented, anybody reading the code would then know how to call it.
There was a problem hiding this comment.
So, the built-in uniqueness check always checks for nulls and excludes null values from the uniqueness check.
The default for the multi-column case (which is the generalized version of expect_column_values_to_be_unique) is to exclude rows where both columns are null from the uniqueness test, unless overridden.
I think I makes sense to check that the value of ignore_row_if is from a known set and error out otherwise.
There was a problem hiding this comment.
Added a check for the 2 expected values. If we want to explicitly add a 3rd case, let's open an issue and separate PR.
There was a problem hiding this comment.
sounds good, i agree with this default behavior
I'll take a look, good call out. If so, I'll open a separate issue/PR though. |
Fixes #200
This fixes the implementation of the
ignore_row_ifparameter inexpect_compound_columns_to_be_uniquehighlighted by @mcannamela in #200 and adds a test todata_test.