[EN .NET] Fix TimexProperty.ToString() to handle DateTimeRanges properly #2894
Conversation
This fixes the conversion to string of DateTimeRange Timex types such as`"(2022-03-15T16,2022-03-15T18,PT2H)"` to not assume there's a part of day in the `TimexProperty` object when there isn't one. Fixes microsoft#2893
|
|
| [TestMethod] | ||
| public void DataTypes_Timex_FromDateTimeRange_ToString() | ||
| { | ||
| var timex = new TimexProperty("(2022-03-15T16,2022-03-15T18,PT2H)"); |
There was a problem hiding this comment.
This is not technicaly correct, as the timex specified doesn't correspond to just one date, but a date range. We need to properly correct this in a more general way.
There was a problem hiding this comment.
Technically what I did here is make it not throw the exception, without changing the original logic of the output - this has always returned incorrect results for a date range, the logic was always wrong for that, but the exception is the issue I'm trying to fix here, because it's very hard to work around it without reimplementing a bunch of stuff on the user side.
I would very much like to have this return the correct output for a date range, that was going to be my next issue to open. Can we fix this exception now and I'll put together a separate PR to fix the output?
There was a problem hiding this comment.
In this case, please add a code comment in that unit test along the lines of:
\\ @TODO Test documenting workaround to avoid exceptions when getting a timex. Proper timex generation fix needed.
Thanks!
There was a problem hiding this comment.
Added the TODO and resolved conflicts 👍
|
@shana, please go ahead and create a new issue for the wrong text generation for ranges. We'll merge once build passes. |
…eTimeRanges (microsoft#2894) * Workaround for TimexProperty.ToString() to not crash on DateTimeRanges * Add TODO for fixing the TimexProperty date range representation properly according to review
This fixes the conversion to string of DateTimeRange Timex types such as
"(2022-03-15T16,2022-03-15T18,PT2H)"to not assume there's a part of day in theTimexPropertyobject when there isn't one.Fixes #2893