Improve date/time hint handling for Postgres based on better testing #146
Conversation
389: tests/integration/***** 381: records_mover/db/postgres/copy_options/date_input_style.py 373: setup.py Reduce total number of bigfiles violations to 1094 or below! Tasks: TOP => quality (See full trace by running task with --trace) Existing violations: 1094 Found 1143 bigfiles violations
| # Supported! | ||
| quiet_remove(unhandled_hints, 'timeonlyformat') | ||
| else: | ||
| cant_handle_hint(fail_if_cant_handle_hint, 'datetimeformat', hints) | ||
| cant_handle_hint(fail_if_cant_handle_hint, 'timeonlyformat', hints) |
There was a problem hiding this comment.
Whoops! This resulted in printing the wrong diagnostic error message.
| # (1 row) | ||
| # | ||
| # postgres=# | ||
| upgrade_date_order_style('DMY', 'dateformat') | ||
| elif dateformat is None: | ||
| # null implies that that date format is unknown, and that the | ||
| # implementation SHOULD generate using their default value and |
There was a problem hiding this comment.
In general you can see this file is very case-based. I think it'd both be less exhausting as a piece of code and more helpful for folks if it were turned into a more dynamic function that didn't need to check every single supported case, but instead did something more dynamic (e.g., "is DD before MM in the string in front of me?). That'd also be one less piece to improve before we can advertise general support for date/time format strings in our hints and loosen the Records Spec accordingly.
| @@ -1 +1 @@ | |||
| 1094 | |||
| 1143 | |||
There was a problem hiding this comment.
See note below about the growth in date_input_style.py and what we can do in the future now that we have better tests...
389: tests/integration/*****
381: records_mover/db/postgres/copy_options/date_input_style.py
373: setup.py
Reduce total number of bigfiles violations to 1094 or below!
Tasks: TOP => quality
(See full trace by running task with --trace)
Existing violations: 1094
Found 1143 bigfiles violations
| datetimeformattz in ['YYYY-MM-DD HH:MI:SSOF', | ||
| 'YYYY-MM-DD HH24:MI:SSOF'] and | ||
| datetimeformat == 'YYYY-MM-DD HH24:MI:SS'): | ||
| datetimeformat in ['YYYY-MM-DD HH24:MI:SS', | ||
| 'YYYY-MM-DD HH:MI:SS']): |
There was a problem hiding this comment.
HH is equivalent to HH24 in our spec.
| self.assertFalse(unhandled_date_cases) | ||
| self.assertFalse(unhandled_datetimeformattz_cases) | ||
| self.assertFalse(unhandled_datetimeformat_cases) | ||
| self.assertFalse(unhandled_timeonly_cases) |
There was a problem hiding this comment.
As the output of this is a function of the interaction of three different hints, I needed to ensure we covered each of the desired cases once, so this will blow up as new test cases are added and we can add hand-validated expectations.
| 'dateformat': "MM-DD-YYYY", | ||
| }, | ||
| 'MDY' | ||
| ), |
There was a problem hiding this comment.
Add more cases, enough so that this test covers each off the cases defined in datetime_cases.py
Based on the new testing approach in #144, this PR adds component tests for date/time types while loading/exporting CSV files in the PostgreSQL database.
Along the way some places for improvement were found!