DM-32407: Fix YAML serialization of empty Timespans #590
Conversation
Codecov Report
@@ Coverage Diff @@
## master #590 +/- ##
==========================================
- Coverage 83.52% 83.52% -0.01%
==========================================
Files 241 241
Lines 30233 30251 +18
Branches 4511 4515 +4
==========================================
+ Hits 25253 25267 +14
- Misses 3786 3788 +2
- Partials 1194 1196 +2
Continue to review full report at Codecov.
|
|
|
||
|
|
||
| # YAML tag name for _SpecialTimespanBound | ||
| _SpecialTimespanBound_yaml_tag = "!lsst.daf.butler._SpecialTimespanBound" |
There was a problem hiding this comment.
I do wonder if we want to have this tag burned into the YAML rather than the notion of EMPTY since the docstring for this class says we should always access it as TimeSpan.EMPTY. Maybe @TallJimbo has thoughts.
There was a problem hiding this comment.
If we are allowed to make it whatever string we want, then yes, lsst.daf.butler.Timespan.EMPTY would be preferable.
If it's not difficult to override the YAML serialization to this extent, it'd arguably be even better to map the entire Timespan to a sentinal string value like "EMPTY", since it's really a property of the entire timespan rather than just one bound, but if it's a lot harder it's not worth the effort.
Do we know what the YAML looks like with this solution?
There was a problem hiding this comment.
The format before/after this change is on the JIRA here: https://jira.lsstcorp.org/browse/DM-32407?focusedCommentId=340215&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-340215
I was not sure that _SpecialTimespanBound is never going to have any other enum values for it, so I decided to keep it open for possible extensions. I was also thinking about replacing whole empty Timespan with a special YAML key, but this would mean adding more knowledge about Timespan details to YamlRepoExportBackend/YamlRepoImportBackend. Right now these two classes only know that Timespan has begin/end timestamps and it works OK for all cases so far. Making YAML representation more complicated adds more logic to those two classes. Alternatively we can define a special YAML representer for Timespan class itself to encapsulate that logic where it belongs, that adds YAML dependency to Timespan.
There was a problem hiding this comment.
Adding a YAML representer to Timespan does feel like the right solution.
There was a problem hiding this comment.
OK, I'll try something quick today on this ticket.
Add special YAML representer and constructor for the Timespan class. Extend unit test with a trivial case for an empty Timespan.
andy-slac
force-pushed
the
tickets/DM-32407
branch
from
200d0be
to
66f3af7
Compare
Add special YAML representer and constructor for the _SpecialTimespanBound
class. Extend unit test with a trivial case for an empty Timespan.
Checklist
doc/changes