Conversation
nhsdevws
left a comment
nhsdevws
left a comment
There was a problem hiding this comment.
Put future date back in the tests.
| today_utc = datetime.now(ZoneInfo("UTC")).date() | ||
| if dt.date() > today_utc: | ||
| # 2. Use Expression Rule Format to parse the date, remove dashes and slashes | ||
| if expressionRule == "%Y%m%d": |
There was a problem hiding this comment.
As below, it seems odd that we only handle YYYYMMDD, YYYY-MM-DD or YYYY-MM-DDT... here. Why not just parse any valid ISO formatted date / datetime? Will check with Paul
There was a problem hiding this comment.
E.g. what about partial dates?
|
|
||
| # Not Empty Validate - Returns exactly what is in the extracted fields no parsing or logic needed | ||
| def _convertToNotEmpty(self, expressionRule, fieldName, fieldValue, summarise, report_unexpected_exception): | ||
| if not fieldValue: |
There was a problem hiding this comment.
Why has this been added? Maybe I'm missing the point of NOTEMPTY, but I'd expect None to be an error (which would already be picked up by the logic below)
There was a problem hiding this comment.
I was kinda thinking that if its empty the overall data from other data sources is also empty this means its an empty data, hence not exactly an error. what would you suggest?
There was a problem hiding this comment.
Might just be a naming issue - convertToNotEmpty makes me think it will never return an empty string, but I might be wrong. Will check with Paul what his intention was. Don't think this is a blocker for getting this MR merged
|
4 participants
Summary
Reviews Required
Review Checklist
ℹ️ This section is to be filled in by the reviewer.