-
Notifications
You must be signed in to change notification settings - Fork 18
Allow some table reordering and ignoring #117
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
Conversation
…it and when not to, because some order matters
Myoldmopar
left a comment
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.
Walkthrough complete.
| @@ -1,6 +1,4 @@ | |||
| #!/usr/bin/env python | |||
| # -*- coding: utf-8 -*- | |||
| from __future__ import unicode_literals | |||
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.
Don't need this with Python 3.
| __copyright__ = "Copyright (c) 2009 Santosh Philip and Amir Roth 2013" | ||
| __license__ = "GNU General Public License Version 3" | ||
|
|
||
| from pathlib import Path |
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.
Start modernizing slightly using Path instances. Could be done lots more places, but I didn't want to grow scope toooo much.
|
|
||
|
|
||
| def thresh_abs_rel_diff(abs_thresh, rel_thresh, x, y): | ||
| def thresh_abs_rel_diff(abs_thresh: float, rel_thresh: float, x: str, y: str) -> tuple[float | str, float | str, str]: |
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.
A little Python type hinting never hurt anyone.
| return 0, 0, 'equal' | ||
| else: | ||
| return '%s vs %s' % (x, y), '%s vs %s' % (x, y), 'stringdiff' | ||
| return f'{x} vs {y}', f'{x} vs {y}', 'stringdiff' |
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.
And more f strings instead of format specifiers
| # Assume we are going to just loop over the rows and compare the data | ||
| search_rows = trows[1:] | ||
|
|
||
| # But we can handle it specially if we passed in table_a and it's just a valid reorder |
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.
Alright, this comment should capture the intent. But basically, if we read table a, then pass it in when we read table b, this code will try to identify matching lines in the table, even if they are out of order. If anything goes awry, diffs are detected, etc., it will revert back to the normal row-by-row comparison on both table a and table b.
| 'Object Count Summary_Entire Facility_Input Fields' | ||
| ] | ||
| if any([x in uheading1 for x in completely_skippable_table_keys]): | ||
| continue |
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.
First off, if a table name is listed here, it can be completely skipped. It won't be included in the equal value count or anything. This should only be used for very specific table names. As of now it is only skipping the input fields table that lists how many autosizable fields and such.
|
|
||
| # create a list of order-dependent table uheading keys, tables that include these keys in the name | ||
| # these will use strict row order enforcement | ||
| row_order_dependent_table_keys = ['Monthly', 'Topology'] |
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.
Here we identify any tables that should enforce row order when doing diffs. Things like topology or time-based should be added here. This can be adjusted as more are identified.
| hdict2, horder2 = table2hdict_horder(table2) | ||
| # but for all other tables, we can use the first table as a baseline to carefully match up the rows | ||
| else: | ||
| hdict2, horder2 = table2hdict_horder(table2, table1) |
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.
And this block will either pass in table2 alone, in which case order dependence will be respected, or pass table1 as an optional second parameter. In that case, table1 order will be used to try to detect row matching, and allow the tables to pass if it's purely a reorder.
| @@ -0,0 +1,209 @@ | |||
| <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN""http://www.w3.org/TR/html4/loose.dtd"> | |||
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.
Several new unit test resource files added for these new behaviors.
| self.assertEqual(334, response[2]) # big diffs | ||
| self.assertEqual(67, response[3]) # small diffs | ||
| self.assertEqual(3763, response[4]) # equals | ||
| self.assertEqual(3756, response[4]) # equals |
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.
Since we aren't checking the input fields count/echo table anymore, these values had to be reduced by that many cells. All good.
|
Looks good over in the E+ branch. Calling it. Merging and tagging v2.1.4. |
The main objectives here were to:
I did a bit of cleanup while in there, and I'll provide a walkthrough of all changes. I added unit test support to cover the new use cases, and updated existing unit tests to cover the behavior where needed.