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

[Python] Add Portuguese support for DateTimeV2 #2876

Merged
merged 8 commits into from
Mar 3, 2022

Conversation

samhickey25
Copy link
Contributor

Add support for datetime entities in python for Portuguese and update the Specs to reflect these changes

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.

Build is failing. Can you take a look? We can review once build passes.

@tellarin
Copy link
Collaborator

tellarin commented Mar 2, 2022

@samhickey25, BTW, I noticed that in the fork where this PR comes from you folks seem to have Python support for Italian Numbers. Are you also looking at contributing that back? That would be very helpful. As well as any other contributions. :-)

@tellarin tellarin changed the title Portuguese python datetime support [Python] Add Portuguese support for DateTimeV2 Mar 2, 2022
@samhickey25
Copy link
Contributor Author

I think we'll get to Italian but at the moment I'm just concentrating on Portuguese support

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 to make.
I also noticed harcoded strings that should be refactored in YAML (along with TODOs). i assume these come as-is from .NET? I'm OK with merging this version.

PortugueseDateTime.Tomorrow) else 0

# handle Date followed by morning, afternoon
# Add handling code to handle morning, afternoon followed by Date
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this replicating the state of the .NET code or a Python-specific workaround?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is replicating .NET behaviour

@@ -695,7 +695,7 @@
},
{
"Input": "Você está livre em 13.5.2015",
"NotSupported": "python",
"NotSupported": "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Such empty NotSupported tags can be removed to keep specs clean.

@@ -1396,7 +1396,7 @@
"Context": {
"ReferenceDateTime": "2016-11-07T00:00:00"
},
"NotSupported": "python",
"NotSupported": "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same.

@@ -883,7 +883,7 @@
},
{
"Input": "Voltarei às 7h01",
"NotSupported": "python",
"NotSupported": "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same.

@samhickey25
Copy link
Contributor Author

Minor changes to make. I also noticed harcoded strings that should be refactored in YAML (along with TODOs). i assume these come as-is from .NET? I'm OK with merging this version.

Yeah these files follow the .NET files and behaviour closely

@@ -54,7 +54,7 @@ def parse_specific_time_of_day(self, source: str, reference: datetime) -> DateTi

# handle Date followed by morning, afternoon
# Add handling code to handle morning, afternoon followed by Date
# Add handling code to handle early/late morning, afternoon
# Add handling code to handle early/late morning, afternoon.
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 for such commits. We can either re-run the same build. Or close/reopen the PR and a new build is triggered.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A new build is already running.

@tellarin tellarin merged commit f603e13 into microsoft:master Mar 3, 2022
Conor-Keaney pushed a commit to purecloudlabs/Recognizers-Text that referenced this pull request Mar 25, 2022
* Add Portuguese DateTime support in Python

* Update Specs with Python changes

* Fix Number import issue

* Update specs according to build and review

* Remove empty NotSupported fields from specs
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