[.NET Timexlib] Implement missing features of TimexProperty.ToString() #2903
Conversation
This implements the missing features of TimexProperty.ToString(), adding support for date ranges, date + duration, etc. This also fixes a bunch of off-by-one bugs when adding days or months to a Timex where it would not take into account potential month/year overflows. It also adds caching of the `Types` hashset of the Timex, so that it doesn't recalculate it every time the property is accessed. Instead, `Types` get cleared whenever a meaningful piece of data in the `TimexProperty` is changed. Some tests have been converted to using `DataRow` instead of inline asserts, so that the test log will show the data being tested. It should also make it easier to see what the tests are doing. There's a lot of static helper methods that have been made into extension methods, because they're already in static classes so there's no overhead in doing it, and it makes caller code shorter and hopefully simpler to read.
| @@ -217,7 +226,7 @@ public static TimexProperty TimexDateAdd(TimexProperty start, TimexProperty dura | |||
| return new TimexProperty | |||
| { | |||
| Year = start.Year, | |||
| Month = (int)(start.Month + duration.Months), | |||
| Month = ((int)(start.Month + duration.Months) % 12) + 1, // months are from 1 to 12 | |||
There was a problem hiding this comment.
I feel like this is still buggy, will add a test for it to tomorrow.
| } | ||
|
|
||
| [TestMethod] | ||
| public void DataTypes_Timex_ToString() |
There was a problem hiding this comment.
What happened to the cases from this one onwards?
There was a problem hiding this comment.
I moved them to TestTimexToString because all the tests before this are pure roundtrip tests and it was getting hard to track what tests tested what codepaths.
| } | ||
|
|
||
| [TestMethod] | ||
| public void DataTypes_Convert_FridayEvening() | ||
| [DataRow("(XXXX-XX-XX,XXXX-XX-XX,P1D)", "1 day")] | ||
| public void DataTypes_Convert_Period(string timex, string expected) |
There was a problem hiding this comment.
It's better to rename to DataTypes_Convert_Undefined_Period or DataTypes_Convert_Undefined_Range for clarity.
| } | ||
|
|
||
| [TestMethod] | ||
| public void DataTypes_Convert_WednesdayToSaturday() | ||
| [DataRow("(2017-09-08T21:19:29,2017-09-08T21:24:29,PT5M)", "8th September 2017 9:19:29PM to 9:24:29PM")] | ||
| public void DataTypes_Convert_Last5Minutes(string timex, string expected) |
There was a problem hiding this comment.
What if the dates are different? Do we have a case for that somewhere for it?
Also, this is technically not "last 5 min". It's just a period of 5 min with those parameters.
And the order in the timex is invalid. Start should be smaller than end.
There was a problem hiding this comment.
I didn't actually do anything with this, I just converted the test to DataRow and added the output that it was missing - the github inline diff is misleading. This is the original code of the test:
[TestMethod]
public void DataTypes_Convert_Last5Minutes()
{
// date + time + duration
var timex = new TimexProperty("(2017-09-08T21:19:29,2017-09-08T21:24:29,PT5M)");
// TODO
}
I didn't change anything in the minute duration to string conversion, so the output here is the same output that it has always had. I agree that the output doesn't seem to match the output of the test, I just added the output of the original test here so I could see what it was doing.
| @@ -93,7 +93,7 @@ public void DataTypes_Helpers_TimeRangeFromTimex() | |||
| public void DataTypes_Helpers_DateFromTimex() | |||
| { | |||
| var timex = new TimexProperty("2017-09-27"); | |||
| var date = TimexHelpers.DateFromTimex(timex); | |||
| var date = TimexHelpers.DateTimeFromTimex(timex); | |||
There was a problem hiding this comment.
Both should return the same results, but the first one is "more correct" as the timex represents a date, not a datetime.
|
Fyi, I've been away for Easter and life and couldn't follow up on this before, I should have more time now to get this thing done, it's not forgotten. |
Thanks for the update, @shana! |
|
@shana Will you still update this PR? Looking forward to merging it. |
Following up on PR #2894 , this implements the missing features of TimexProperty.ToString(), adding support for date ranges, date + duration, etc.
This also fixes a bunch of off-by-one bugs when adding days or months to a Timex, where it would not take into account potential month/year overflows.
It also adds caching of the
Typeshashset of the Timex, so that it doesn't recalculate it every time the property is accessed. Instead,Typesget cleared whenever a meaningful piece of data in theTimexPropertyis changed.Some tests have been converted to using
DataRowinstead of inline asserts, so that the test log will show the data being tested. It should also make it easier to see what the tests are doing. I also moved ToString tests to a separate class from the Roundtrip tests so it's easier to follow.There's a lot of static helper methods that have been made into extension methods, because they're already in static classes so there's no overhead in doing it, and it makes caller code shorter and hopefully simpler to read. This has the side effect of making these
DateTimeextensions methods visible to consumers that include the TimexExpression namespace in the code - I don't know if that's undesirable or not, let me know if it's not something you want.Fixes #2904