Overhaul how date & time parsing works. #52
Conversation
mikix
changed the title
Sample/fhirdate.py
Outdated
|
|
||
| class _FHIRDateTimeMixin: |
There was a problem hiding this comment.
I wanted to share a lot of code between these classes, but I didn't really want to imply a hierarchy between them - a Time and a Date field really don't have any overlap or ancestry. They might share some properties / methods - but mostly because they share those with all the FHIR classes.
So I chose a private mixin (really, same as a private superclass, but labeled a mixin to make the intent clear -- this is just sharing some code). I could have maybe done this a different way? Open to suggestions.
But ideally the usage is clear to consumers: don't reference this mixin, it's just implementation details.
There was a problem hiding this comment.
In fact, there's probably an argument that each of these classes should go in their own file (fhirdatetime.py, fhirinstant.py etc) just like each model. But because I wanted to share this code, I left them all here. But I'm open to splitting the files.
There was a problem hiding this comment.
i do :sort of: like the ability to say 'is this object a time related object', but I buy the mixin version.
I do think it might help with readability to have each class in separate files, or to move just the mixin to its own file.
There was a problem hiding this comment.
We talked out-of-band and are going to keep the mixin and also move these into separate files. (already done in the latest commit of this PR)
| ################################## | ||
|
|
||
| # Pulled from spec for date | ||
| _REGEX = re.compile(r"([0-9]([0-9]([0-9][1-9]|[1-9]0)|[1-9]00)|[1-9]000)(-(0[1-9]|1[0-2])(-(0[1-9]|[1-2][0-9]|3[0-1]))?)?") |
Sample/fhirdate.py
Outdated
|
|
||
| class _FHIRDateTimeMixin: |
There was a problem hiding this comment.
i do :sort of: like the ability to say 'is this object a time related object', but I buy the mixin version.
I do think it might help with readability to have each class in separate files, or to move just the mixin to its own file.
This commit breaks FHIRDate into four classes: - FHIRDate - FHIRDateTime - FHIRInstant - FHIRTime BREAKING CHANGES: - Obviously, some previously-FHIRDate fields will now parse as FHIRDateTime, FHIRInstant, or FHIRTime fields instead (as appropriate). - These new classes have different field names for the python object version of the JSON value. They use `.datetime` or `.time` instead of `.date`. 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 Python object field is now always the appropriate type and name (FHIRDate.date is datetime.date, FHIRDateTime.datetime and FHIRInstant.datetime are datetime.datetime, and FHIRTime.time is datetime.time. 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 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.
This commit breaks FHIRDate into four classes:
BREAKING CHANGES:
.datetimeor.timeinstead of.date.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.datetimefield 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.)This is a sister PR (one which does not need to maintain backwards compatibility) to a similar downstream fhirclient refactor.