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

add support for Japanese (JA) #171

Merged

Conversation

siikamiika
Copy link
Contributor

@siikamiika siikamiika commented Jun 7, 2018

Changes proposed in this pull request:

  • Add support for JA

Status

  • READY
  • HOLD
  • WIP (Work-In-Progress)

How to verify this change

import num2words
num2words.num2words(123456, lang='ja')
num2words.num2words(123456, lang='ja', reading=True)
num2words.num2words(-5, lang='ja')
num2words.num2words(0, lang='ja', reading=True)
num2words.num2words(74, lang='ja', reading=True)
num2words.num2words(74, lang='ja', reading=True, prefer=['し', 'しち'])
num2words.num2words(123456, lang='ja', reading=True, to="ordinal")
num2words.num2words(123456, lang='ja', reading=True, to="ordinal_num")

Additional notes

@coveralls
Copy link

coveralls commented Jun 12, 2018

Coverage Status

Coverage increased (+0.2%) to 89.805% when pulling d96b828 on siikamiika:japanese-language-support into b492530 on savoirfairelinux:master.

@erozqba
Copy link
Collaborator

erozqba commented Jun 12, 2018

Hi @siikamiika
Thanks a lot for taking the time to make this PR! It's really appreciated.
I don't know any Japanese so we really trust the contributors that make the PR.
I can see this is a work in progress, as a suggestion, don't forget to update the README.rst and to implement as many automatic tests as you can.

Thanks a lot again!

@ventilooo
Copy link
Contributor

hey, @erozqba ! I speak a bit of Japanese.
I'll try to handle the review as much as I can 😉

@siikamiika ping me once the WIP is over !
Thanks again for your contribution 👍

@ventilooo ventilooo self-requested a review June 12, 2018 14:59
@siikamiika
Copy link
Contributor Author

Seems like the code is having some issues with Python 2. Probably Unicode related. The tests should be fine for Python 3 now, but I haven't updated README yet.

@siikamiika
Copy link
Contributor Author

@erozqba @ventilooo Everything looks fine to me now. This sure was an interesting language to implement. 😅 I'm also just learning Japanese so I did this as an exercise. I did some research on the preferred readings of some number words in Japanese, but there could be errors somewhere.

Copy link
Collaborator

@erozqba erozqba left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @siikamiika for this contribution!
@ventilooo thanks in advance for your code review.

(724, ("神亀", "じんき")),
(729, ("天平", "てんぴょう")),
# (749, ("天平感宝", "てんぴょうかんぽう")),
(749, ("天平勝宝", "てんぴょうしょうほう")),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why there are some commented lines as this one in this list?

@siikamiika
Copy link
Contributor Author

siikamiika commented Jun 14, 2018 via email

Copy link
Contributor

@ventilooo ventilooo left a comment

Choose a reason for hiding this comment

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

@siikamiika Instead of putting some emperor/empress in comment, could you solve this with a method ?

Like with ⬇️

self.assertEqual(n2j(0, prefer=["〇"]), "〇")

In order to easily give the choice to the user.

Also I don't know how useful this could be, but having the option to choose the the output format between

If I want to write an app for kid, they don't necessarily know all the kanji associated with numbers.

Or if I want to write an app for foreigner, to learn Japanese. Having an romaji output would help.

@siikamiika
Copy link
Contributor Author

@ventilooo

Instead of putting some emperor/empress in comment, could you solve this with a method ?

Like with ⬇️

self.assertEqual(n2j(0, prefer=["〇"]), "〇")

In order to easily give the choice to the user.

Sounds like a good idea, I'll try to do that.

Also I don't know how useful this could be, but having the option to choose the the output format between

  • Romaji
  • Kanji
  • Hiragana

If I want to write an app for kid, they don't necessarily know all the kanji associated with numbers.
Or if I want to write an app for foreigner, to learn Japanese. Having an romaji output would help.

Hiragana output is currently supported with the reading flag, but I'm not sure if that's the best way to choose it if you want to choose output between multiple different writing systems. There's also katakana, which would be pretty easy to implement if I remember the Unicode code points correctly.

Romaji is a bit complicated, though. First, there are multiple ways to romanize Japanese text, as seen from your link. An optional dependency to something like python-romkan could be added, but even then there would be the problem of separating the number words with spaces only for romanized Japanese but not with kana. Anyway, I agree that there could be some use for that feature. I guess I could look into implementing romaji at some point.

@siikamiika
Copy link
Contributor Author

I just realized that the era thing wasn't as simple as having some ruler for less than a year. The prefer suddenly makes a lot more sense but I might replace the year in the tuples with another tuple containing both the start and the end of the era.

Some reading:
https://en.wikipedia.org/wiki/Eiwa
https://en.wikipedia.org/wiki/Tenju

Copy link
Contributor

@ventilooo ventilooo left a comment

Choose a reason for hiding this comment

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

That's perfect !
Once again @siikamiika , thanks for your contribution !

If you want to discuss further on future improvement for the Japanese support.

Just open an issue with the RFC tag 😉 🤞

@ventilooo ventilooo merged commit 89554ff into savoirfairelinux:master Jun 15, 2018
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.

None yet

4 participants