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

Improve Arabic implementation #176

Merged
merged 12 commits into from Sep 11, 2018
Merged

Conversation

alhazmy13
Copy link
Contributor

@alhazmy13 alhazmy13 commented Jul 3, 2018

Changes proposed in this pull request:

  • Improve Arabic implementation
  • Support SAR, EGP and KWD currency.

Status

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

How to verify this change

Run test_ar.py tests

Additional notes

What's the problem with current implementation?

A lot and a lot of grammar mistakes, Arabic language is very complicated, and when it comes to converting numbers to words, I think it is the most difficult language because it has so many rules that depend on the “number” state and the “counted” state.

For example:

The number one & two matches the counted in its feminine state, where the numbers from 3 to 10 are contrary to the counted, etc.

What's new?

This implementation has the ability to determine a digit feminine status using its group level & Currency Name Feminine field status.

The whole conversion process depends on dividing the number into Group Levels. Each group contains 3 digits & these levels are numbered as in this example:

Number: 786,501,652.623
623: Group Level -1
652: Group Level 0 
501: Group Level 1
786: Group Level 2 

Also, the Arabic language is sensitive to currency names which also depends on the number state, so I had to add four different names for the same currency name to support all cases. Their fields are used at the end of the method when constructing the final string.

I'll write a wiki page later when you approve and merge this.

@coveralls
Copy link

coveralls commented Jul 3, 2018

Coverage Status

Coverage increased (+0.8%) to 91.301% when pulling d22dfc5 on alhazmy13:master into 7639f5a on savoirfairelinux:master.

@coveralls
Copy link

coveralls commented Jul 3, 2018

Coverage Status

Coverage increased (+0.009%) to 90.752% when pulling cb6502b on alhazmy13:master into f275d0f on savoirfairelinux:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 90.752% when pulling cb6502b on alhazmy13:master into f275d0f on savoirfairelinux:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 90.752% when pulling cb6502b on alhazmy13:master into f275d0f on savoirfairelinux:master.

@ventilooo ventilooo added this to In progress in Release via automation Jul 3, 2018
@ventilooo ventilooo added this to the 1.0 milestone Jul 3, 2018
@ventilooo
Copy link
Contributor

Hi @alhazmy13,

Thanks for your PR

It's still in Work In Progress. Did you plan to finish it ?

@alhazmy13
Copy link
Contributor Author

Hi @ventilooo , Yes it's ready to merge it with master branch

@ventilooo ventilooo merged commit f72c999 into savoirfairelinux:master Sep 11, 2018
Release automation moved this from In progress to Done Sep 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants