Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EN .NET] Fix TimexProperty.ToString() to handle DateTimeRanges properly #2894

Merged
merged 3 commits into from
Mar 14, 2022
Merged

[EN .NET] Fix TimexProperty.ToString() to handle DateTimeRanges properly #2894

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
83e9888
[EN .NET] Fix TimexProperty.ToString() to handle DateTimeRanges properly
shana Mar 11, 2022
54f5755
Add TODO for fixing the TimexProperty date range representation properly
shana Mar 14, 2022
3118eca
Merge master into fixes/2893-timex-tostring
shana Mar 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .NET/Microsoft.Recognizers.Text.DataTypes.DataDrivenTests/TestTimex.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,13 @@ public void DataTypes_Timex_FromTime()
Assert.AreEqual("T23:59:30", TimexProperty.FromTime(new Time(23, 59, 30)).TimexValue);
}

[TestMethod]
public void DataTypes_Timex_FromDateTimeRange_ToString()
{
var timex = new TimexProperty("(2022-03-15T16,2022-03-15T18,PT2H)");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the TODO and resolved conflicts 👍

Assert.AreEqual("15th March 2022 4PM", timex.ToString());
}

private static void Roundtrip(string timex)
{
Assert.AreEqual(timex, new TimexProperty(timex).TimexValue);
Expand Down
23 changes: 21 additions & 2 deletions .NET/Microsoft.Recognizers.Text.DataTypes.TimexExpression/English/TimexConvertEnglish.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Globalization;

namespace Microsoft.Recognizers.Text.DataTypes.TimexExpression
Expand Down Expand Up @@ -213,9 +214,27 @@ private static string ConvertDateTime(TimexProperty timex)

private static string ConvertDateTimeRange(TimexProperty timex)
{
if (timex.Types.Contains(Constants.TimexTypes.TimeRange))
var parts = new List<string>();

var types = timex.Types;
if (types.Contains(Constants.TimexTypes.Date))
{
parts.Add(ConvertDate(timex));
}

if (types.Contains(Constants.TimexTypes.Time))
{
parts.Add(ConvertTime(timex));
}

if (timex.PartOfDay is not null)
{
parts.Add(ConvertTimeRange(timex));
}

if (parts.Count > 0)
{
return $"{ConvertDate(timex)} {ConvertTimeRange(timex)}";
return string.Join(" ", parts);
}

// date + time + duration
Expand Down