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 German support for recognizers-date-time #2728

Closed

Conversation

BrianInGermany
Copy link

@BrianInGermany BrianInGermany commented Oct 12, 2021

  • I converted the German Datetime YAML into a python file and attempted to adapt the English config files to the german patterns.
  • I ran the pytest in the Python/tests directory and everything passed, except 8 tests, which were skipped.
  • A few of the configs had to be commented out because I could not identify the as being supported in German
  • The German holidays different than English ones are not all registered correctly and are thus not all recognizing or resolving correctly.
  • Looking forward to feedback.

#2197

@ghost
Copy link

ghost commented Oct 12, 2021

CLA assistant check
All CLA requirements met.

@BrianInGermany
Copy link
Author

BrianInGermany commented Oct 13, 2021

I see there are some issues here... I will have a look, putting on DRAFT

@BrianInGermany BrianInGermany marked this pull request as draft October 13, 2021 16:31
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 see a lot more than 8 tests are now failing. What were the main change's you've made?
I'd suggest first making the Python version run, instead of editting the YAML file at the same time. Directly porting the .NET code should lead to all tests passing (please make sure the json schema tests are enabled for Python). I.e., you should not copy the English Python code and adapt it (well, you can do it as a base, but then reference .NET back for the latest flow).
Also, you mentioned you "converted the German Datetime YAML into a python file and attempted to adapt the English config files to the german patterns." I believe in the current version it's being auto-generated, as I see edits in the generator for German, as well as the auto-gen header in the German resources file. Please confirm that the auto generation is running correctly.

@BrianInGermany
Copy link
Author

@tellarin
Thanks for your quick reply.
Initially I didnt understand that the python regexes were autogenerated from the YAML. I did not disable the auto generation.

Afterwards, I made a first change to the order of the yaml, because the autogeneration of the python regexes did not work due to simple patterns being referenced before they were declared in the YAML.

Regarding the failing German number tests, these must have been failing before I started, because I did not touch any number patterns or python config files.

Thanks for the suggestion about porting the .NET code.

It seems my pull request was premature, I am trying to better understand how everything works. Would you suggest I close it for now, or do you mind if I leave it as a draft for a little while?

Cheers

Brian

@tellarin
Copy link
Collaborator

@BrianInGermany feel free to leave it in Draft for as long as you need. You can also use it to ask any questions you may have.
The main README in the repo has three links to old blog posts that talks about the overall process. They are a little outdated, but may still be useful.

@tellarin
Copy link
Collaborator

@BrianInGermany Are you still looking into this? I suggest cleaning the PR, so we can start by merging the basic code skeleton for German and its tests. Then implement and refine sub-type by sub-type.

@tellarin
Copy link
Collaborator

tellarin commented May 5, 2022

Closing this as #2914 has added German Number/Currency/DateTimeV2 close to parity with .NET.

@tellarin tellarin closed this May 5, 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