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.

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

[.NET] Integration of new mode termed "TasksMode" in DatetimeOption.cs #2972

Merged
merged 15 commits into from
Jun 7, 2022

Conversation

Neelisha-saxena
Copy link
Contributor

TasksMode: When the input text is in the form of tasks (i.e., Schedule 1:1 call on Monday), this mode modifies the value of datetime reference of the input text.

Implemented four-digit number handling rule under Tasksmode:
Should not treat a four-digit number as a "daterange" if the input text does not include other reference of month or year. It should not treat 2005 as a daterange in statements like "Total no of milk packet is 2005."

Copy link
Collaborator

@tellarin tellarin left a comment

Choose a reason for hiding this comment

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

It seems this PR is also altering the default behaviour (without the new flag), which should not happen.
No need to close the PR and open a new one, you can just push the changes to the branch that originated the PR.
Thanks.

@@ -38,6 +38,11 @@ public enum DateTimeOptions
/// </summary>
NoProtoCache = 16,

/// <summary>
/// NoProtoCache
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please update comments

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd also personally prefer to use 2^20 as the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any specific reason for using 2^20 value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's the highest available value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The file doesn't seem updated here.

@tellarin
Copy link
Collaborator

tellarin commented Jun 6, 2022

@Neelisha-saxena Build passed. So now only the new mirror DateTimeModel.json specs are missing.
For the case(s) where the default mode produce false positives (like in "2000 pieces"), mark it as NotSupported in .NET
and add a Comment attribute with a message saying the default more produces a false positive for it and that it's a case from TasksMode.
Once we have the specs, we can merge this.
Thanks!

@Neelisha-saxena
Copy link
Contributor Author

Neelisha-saxena commented Jun 6, 2022

I have implemented the suggestions.

Copy link
Collaborator

@tellarin tellarin left a comment

Choose a reason for hiding this comment

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

Minor changes left.

@@ -18407,7 +18407,7 @@
"Context": {
"ReferenceDateTime": "2019-12-15T01:00:00"
},
"NotSupported": ".NET, javascript, python, java",
"NotSupported": "dotnet, javascript, python, java",
"Results": [
{
"Text": "2000",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be empty as the real result should be nothing.

@@ -38,6 +38,11 @@ public enum DateTimeOptions
/// </summary>
NoProtoCache = 16,

/// <summary>
/// NoProtoCache
Copy link
Collaborator

Choose a reason for hiding this comment

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

The file doesn't seem updated here.

It should not treat 2005 as a daterange in statements like "Milk 2005."
(The year 2005 should be treated as a number only.)
*/
private bool ShouldSkipOnlyYear(ExtractResult er)
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to do this in this PR, but it would be interesting if this mode-specific behaviours/code is isolated into it's own module/strategy.
Please move it along with new changes in a followup PR.

Copy link
Collaborator

@tellarin tellarin left a comment

Choose a reason for hiding this comment

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

I've editted the two remaining files.

@tellarin tellarin merged commit 41a5f12 into microsoft:master Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants