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 kannada language support #243

Merged
merged 22 commits into from May 12, 2019
Merged

Conversation

akki2825
Copy link
Contributor

@akki2825 akki2825 commented Feb 12, 2019

Fixes # by...

Changes proposed in this pull request:

Adds Kannada language support.

Status

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

@akki2825
Copy link
Contributor Author

@erozqba tests are failing for test_cli.py. could you guide me here?

@kkonieczny
Copy link
Contributor

kkonieczny commented Feb 13, 2019

@akki2825 can you update your branch?

@akki2825
Copy link
Contributor Author

@kkonieczny thanks for the reply. I did update, its passing on py36 env but failing on others. Not sure what I'm missing missing.

@@ -101,8 +102,6 @@ def num2words(number, ordinal=False, lang='en', to='cardinal', **kwargs):
if lang not in CONVERTER_CLASSES:
raise NotImplementedError()
converter = CONVERTER_CLASSES[lang]
if isinstance(number, str):
number = converter.str_to_number(number)
Copy link

Choose a reason for hiding this comment

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

Hey @akki2825 you are missing this section. With this conditional, the tests pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @lepisma

@coveralls
Copy link

coveralls commented Apr 14, 2019

Pull Request Test Coverage Report for Build 698

  • 56 of 59 (94.92%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 92.968%

Changes Missing Coverage Covered Lines Changed/Added Lines %
num2words/lang_KN.py 33 36 91.67%
Totals Coverage Status
Change from base Build 683: 0.002%
Covered Lines: 4933
Relevant Lines: 5214

💛 - Coveralls

@akki2825 akki2825 force-pushed the master branch 2 times, most recently from ebb04b2 to 8e2677a Compare April 14, 2019 10:35
@sarahberanek
Copy link
Contributor

I am very much interested in this. How is the status of this pull request?

@lepisma
Copy link

lepisma commented May 2, 2019

@sarahberanek I don't know kannada but I believe there were/are certain inaccuracies in few numbers or number ranges. We use a fork (not PRed here) which probably has those fixed. @akki2825 can you check this fork's code and say something about the changes?

@sarahberanek
Copy link
Contributor

@lepisma ah that is interesting! Thank you for the info. Any way that we can fix the bugs here and merge a correct version into the official master?

@akki2825
Copy link
Contributor Author

akki2825 commented May 3, 2019

@lepisma thanks for the info. I have missed a condition in number ranges. @sarahberanek, I'll make the necessary changes and get back by tomorrow.

@sarahberanek
Copy link
Contributor

Super, thanks!

@sarahberanek
Copy link
Contributor

Because there might be a new release this week of num2words, can we include this into the master? :-) Highly appreciated!

@erozqba
Copy link
Collaborator

erozqba commented May 10, 2019

@akki2825 Thanks a lot for this contribution! I have tried to resolve a conflict on your PR, but my changes were not correct. Please check Travis and Coveralls responses. Could you also add more tests? You could check on Travis the missed lines.

I will be doing a new release this weekend, it will be great to include this on the release.

@erozqba
Copy link
Collaborator

erozqba commented May 11, 2019

@akki2825 I will do a release tomorrow, do you think you will have time to finish this before the release so we can make @sarahberanek happy?

@erozqba
Copy link
Collaborator

erozqba commented May 12, 2019

@akki2825 thanks a lot! We are almost there just missing a couple of tests to cover these lines https://coveralls.io/builds/23334032/source?filename=num2words%2Flang_KN.py
I can't merge if Coveralls is failing, 🙏

@akki2825
Copy link
Contributor Author

akki2825 commented May 12, 2019

Hey, I was a bit occupied. Apologies for the delay. @erozqba. you could merge it now.

@erozqba
Copy link
Collaborator

erozqba commented May 12, 2019

@akki2825 I forgot to mention before, could you update the README.md to add this new language as part of your PR?

@akki2825
Copy link
Contributor Author

@erozqba done.

@erozqba erozqba merged commit cbfc8fd into savoirfairelinux:master May 12, 2019
@eortiz-tracktik eortiz-tracktik mentioned this pull request May 12, 2019
@sarahberanek
Copy link
Contributor

@akki2825 I will do a release tomorrow, do you think you will have time to finish this before the release so we can make @sarahberanek happy?

Haha, yes thanks @akki2825 and @erozqba! I am happy now 😄

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

6 participants