Overhaul how date & time parsing works. #164
Conversation
| # Generated from FHIR {{ info.version }} ({{ profile.url }}) on {{ info.date }}. | ||
| # {{ info.year }}, SMART Health IT. | ||
| # Generated from FHIR {{ info.version }} ({{ profile.url }}). | ||
| # {{ info.year }}, SMART Health IT. |
There was a problem hiding this comment.
I figured including the timestamp of generation made reviews so noisy, it wasn't worth it. Year I'll keep for copyright reasons. (Though I'm not 100% on the law around copyright of these generated files necessarily being the year of generation... but whatever, not my lane.)
There was a problem hiding this comment.
could i talk you into subbing in the version of the fhir client? is that useful?
There was a problem hiding this comment.
Hmm OK but... the version of fhirclient at generation time might not be the version at release time (i.e. we'd have to add a release-time check item to regenerate models with the correct version).
As for whether it is useful... it's got a non-zero utility. The version could be seen in __init__.py a folder up, but that doesn't mean it's bad to repeat it here (except for the risk of being stale, as above).
What's your vibe on the tradeoff? We could add a unit test to check the comment against the version so they don't go stale...
| from . import fhirdate | ||
| from . import fhirdate |
There was a problem hiding this comment.
I think the generating code could do something smart to avoid this double import, but I didn't bother.
There was a problem hiding this comment.
leave that for a 'move the imports to the top' kind of PR
mikix
force-pushed
the
mikix/dates
branch
3 times, most recently
from
4791f9f
to
7cc26f2
Compare
mikix
force-pushed
the
mikix/dates
branch
2 times, most recently
from
c9ca8d6
to
bd88acc
Compare
mikix
changed the title
| # Generated from FHIR {{ info.version }} ({{ profile.url }}) on {{ info.date }}. | ||
| # {{ info.year }}, SMART Health IT. | ||
| # Generated from FHIR {{ info.version }} ({{ profile.url }}). | ||
| # {{ info.year }}, SMART Health IT. |
There was a problem hiding this comment.
could i talk you into subbing in the version of the fhir client? is that useful?
| from . import fhirdate | ||
| from . import fhirdate |
There was a problem hiding this comment.
leave that for a 'move the imports to the top' kind of PR
This commit breaks FHIRDate into four classes: - FHIRDate - FHIRDateTime - FHIRInstant - FHIRTime BREAKING CHANGES: - If consuming code were inspecting the elementProperties array and doing a check like `element_type is FHIRDate`, that will now fail for `datetime`, `instant`, and `time` elements. Backwards compatibility is however maintained for checks written like `issubclass(element_type, FHIRDate)`. Instances have a similar resolution: `isinstance(obj, FHIRDate)` will still work. - If consuming code were manually creating FHIRDate objects themselves with a time component, that will now fail with a ValueError. Since the first item is unavoidable if we want to fix the bugs listed below and has a workaround that works before and after this change, and the second item is not an expected workflow, I hope that such breaking changes do not cause too much harm for consumers. BUG FIXES: - FHIR `time` fields are now correctly parsed. Previously, a time of "10:12:14" would result in a **date** of "1001-01-01" - Passing too much detail to a `date` field or too little detail to an `instant` field will now correctly throw a validation error. For example, a Patient.birthDate field with a time. Or an Observation.issued field with just a year. - Sub-seconds would be incorrectly chopped off of a `datetime`'s `.isostring` (which the FHIR spec allows us to do) and an `instant`'s `.isostring` (which the FHIR spec **does not** allow us to do). The `.date` Python representation and the `.as_json()` call would both work correctly and keep the sub-seconds. Only `.isostring` was affected. IMPROVEMENTS: - Leap seconds are now half-supported. The FHIR spec says clients "SHOULD accept and handle leap seconds gracefully", which we do... By dropping the leap second on the floor and rolling back to :59. But this is an improvement on previous behavior of a validation error. The `.as_json()` value will still preserve the leap second. - The `.date` field is now always the appropriate type (datetime.date for FHIRDate, datetime.datetime for FHIRDateTime and FHIRInstant, and datetime.time for FHIRTime). Previously, a `datetime` field might result in a datetime.date if only given a date portion. (Which isn't entirely wrong, but consistently providing the same data type is useful.) - The new classes have appropriately named fields in addition to the backwards-compatible `.date` field -- FHIRDateTime.datetime, FHIRInstant.datetime, and FHIRTime.time. These will always be the same value as `.date` for now - but in a future major release, the `.date` alias may be dropped. - The dependency on isodate can now be dropped. It is lightly maintained and the stdlib can handle most of its job nowadays. - Much better class documentation for what sort of things are supported and which are not.
Note: I want to prepare a simultaneous PR for fhir-parser, since it will need the same sort of fixes. But that version does not need to maintain backwards compatibility, so it will be a useful comparison for how we might want this to look when we next bump major versions. That's coming shortly.
This commit breaks FHIRDate into four classes:
BREAKING CHANGES:
element_type is FHIRDate, that will now fail fordatetime,instant, andtimeelements. Backwards compatibility is however maintained for checks written likeissubclass(element_type, FHIRDate). Instances have a similar resolution:isinstance(obj, FHIRDate)will still work.Since the first item is unavoidable if we want to fix the bugs listed below and has a workaround that works before and after this change, and the second item is not an expected workflow, I hope that such breaking changes do not cause too much harm for consumers.
BUG FIXES:
timefields are now correctly parsed. Previously, a time of "10:12:14" would result in a date of "1001-01-01"datefield or too little detail to aninstantfield will now correctly throw a validation error. For example, a Patient.birthDate field with a time. Or an Observation.issued field with just a year.datetime's.isostring(which the FHIR spec allows us to do) and aninstant's.isostring(which the FHIR spec does not allow us to do). The.datePython representation and the.as_json()call would both work correctly and keep the sub-seconds. Only.isostringwas affected.IMPROVEMENTS:
.as_json()value will still preserve the leap second..datefield is now always the appropriate type (datetime.date for FHIRDate, datetime.datetime for FHIRDateTime and FHIRInstant, and datetime.time for FHIRTime). Previously, adatetimefield might result in a datetime.date if only given a date portion. (Which isn't entirely wrong, but consistently providing the same data type is useful.).datefield -- FHIRDateTime.datetime, FHIRInstant.datetime, and FHIRTime.time. These will always be the same value as.datefor now - but in a future major release, the.datealias may be dropped.This is a sister PR (one which maintains some level of backwards compatibility) to the upstream more drastic refactor in fhir-parser.
Fixes: #32
Fixes: #11