-
Notifications
You must be signed in to change notification settings - Fork 84
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
chore: refactor compare_dicts
#1224
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks @EdAbati I like this a lot more! I left a comment but can also be thought as a follow up.
For the naming, I think this is the closest we have to what in pandas and polars is assert_frame_equal
, but we are not there quite yet, so I am not sure π
else: | ||
assert lhs == rhs, (lhs, rhs) | ||
are_valid_values = lhs == rhs | ||
assert are_valid_values, f"Mismatch at index {i}: {lhs} != {rhs}\nExpected: {expected}\nGot: {result}" |
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.
Should we record the full diff instead of stopping at the first unequal encounter?
What type of PR is this? (check all applicable)
Related issues
Checklist
If you have comments or can explain your changes, please do so below.
I propose a couple of changes:
pandas
, I'm printing all the values of the columns that differ and the index where the difference was first noticed. This is particularly helpful for dataframes libraries that don't keep row order (* cough cough * pyspark * cough cough *)Before:
After:
compare_dicts
because we are not actually comparing 2 dictionaries. π I propose something likeassert_equal_data
, any better name? This makes the diff of the PR a bit large, happy to revert if you think we should keep the previous name