-
Notifications
You must be signed in to change notification settings - Fork 79
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
Implement Azerbaijani support #212
base: master
Are you sure you want to change the base?
Conversation
Hello, @saymantech-co, thank you for helping with the Mycroft project! We welcome everyone To protect yourself, the project, and users of Mycroft technologies we require Please visit https://mycroft.ai/cla to initiate this one-time signing. Thank |
Signed the cla. |
1dac5d9
to
6ee8412
Compare
@krisgesling For the date time stuff to work, the "az_AZ" locale needs to be generated in the OS, that's why the tests failed. and it can't be done from inside the code, it requires root access. Is this acceptable or should I find another way to handle the date time stuff? |
Sorry I missed the notification for your last message. My first reaction is that we should expect the distribution packaging this to ensure the correct locales are available, and yes we can add that to the CI job here too. I'm not at all familiar with the locale module so not sure what side effects we might be creating by switching the locale back and forth. There are also warnings of it not being threadsafe. Would it be sufficient for us to install |
#216 looks like it generated the locale without issue, so we can try it but I'm very interested to hear from anyone who's worked with locale before. Or if there are other options we can go with. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey sorry it's taken a while to get to this.
It's always a challenge reviewing extensive code in another language! But it looks pretty great.
There are a few little questions below but nothing of concern. The only thing is whether we really need to set the locale or not.
test/test_format_az.py
Outdated
# value is platform dependent so better not use in tests? | ||
# self.assertEqual( | ||
# pronounce_number(sys.float_info.min), "iki nöqtə iki iki times " | ||
# "on to the power of " | ||
# "mənfi üç yüz " | ||
# "və səkkiz") | ||
# self.assertEqual( | ||
# pronounce_number(sys.float_info.max), "bir nöqtə yeddi doqquz " | ||
# "times on to the power of" | ||
# " üç yüz və səkkiz") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd agree with the comment - this can be deleted unless you see specific value in it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, most of these comments are the result of using the english files as template. I'll review and remove them.
test/test_format_az.py
Outdated
self.assertEqual( | ||
pronounce_number(1000000000000, short_scale=True), | ||
"bir trilyon") | ||
# TODO maybe beautify this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this relevant to the AZ test or just cut and pasted from the other file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, sorry I used english files as template and edited them, and seems i have forgotten to remove some of non relevant comments. I'll review and remove them.
test/test_parse_az.py
Outdated
@@ -0,0 +1,429 @@ | |||
# | |||
# Copyright 2017 Mycroft AI Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to use the current year in each of these license headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will fix them.
The reason for switching the locale back and forth was when running pytest, after running AZ tests, because of changing the locale in AZ tests, all other language tests failed, So in AZ, I returned the locale to the previous locale. if that is not necessary and only passing test_parse_az and test_format_az is enough, the switching back and forth is not required and can be removed. |
With the AZ language pack installed and all the locale setting removed from testExtract("3 avqustda anama zəng etməyi xatırlat",
"2017-08-03 00:00:00", "anama zəng etməyi xatırlat")
testExtract("iyulun 4 də atəşfəşanlıq al",
"2017-07-04 00:00:00", "atəşfəşanlıq al")
testExtract("dekabr 3",
"2017-12-03 00:00:00", "")
testExtract("5 iyun 2017 ci il axşamı anama zəng etməyi xatırlat",
"2017-06-05 19:00:00", "anama zəng etməyi xatırlat") Is that what you're seeing also? Are these the only test cases that would rely on the locale getting set or does it point to something else going on? |
yeah, These are the cases that rely on locale being set. |
fix license year
988b2ae
to
d449931
Compare
@krisgesling is using the locale setting alright or should I try another way to handle these cases? |
I'd really like it if we could find another way. But I don't have a suggestion for what that is sorry. |
@krisgesling, Done, no locale setting required for parsing datetime. |
extract datetime without setting locale (cherry picked from commit 575299c) remove extra comments and imports fix license year (cherry picked from commit d449931) improve az-az duration extract (cherry picked from commit 6ee8412) Implemented Azerbaijani support (cherry picked from commit 0439a80) Co-authored-by: Siavash Mollayi <[email protected]>
Implement Azerbaijani support(az_AZ)
Type of PR
Testing