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

Convert strings to Decimal values #223

Merged
merged 2 commits into from Jan 18, 2019

Conversation

btharper
Copy link
Contributor

@btharper btharper commented Nov 1, 2018

Fixes #182 by coercing strings into numeric objects

Changes proposed in this pull request:

  • Punt string handling to python Decimal object, this correctly represents both integers and floats (except with regards to trailing zeros)
    • num2words('1.00') == 'one' instead of 'one point zero zero'
  • Change command line tests to reflect handling of ints

Status

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

How to verify this change

"bin/num2words 1" yields 'one' instead of 'one point zero'
all((num2words(i) == num2words(str(i)) for i in range(1000))) == True

Additional notes

String to Decimal conversion is available in the language converter object, I'm unsure if this will ever need to be adapted per language, and it seems ill-advised to guess based on current or target locale/language.
Python >= 3.5 can use underscores as thousands separators in strings i.e. num2words('3_000') = 'three thousand'

Punt string handling to python Decimal object, this correctly represents both
integers and floats (except with regards to trailing zeros)
Change command line tests to reflect handling of ints
@coveralls
Copy link

coveralls commented Jan 18, 2019

Pull Request Test Coverage Report for Build 615

  • 4 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 92.611%

Totals Coverage Status
Change from base Build 614: 0.006%
Covered Lines: 4762
Relevant Lines: 5048

💛 - Coveralls

@kkonieczny kkonieczny merged commit c1292c1 into savoirfairelinux:master Jan 18, 2019
@shulcsm
Copy link
Contributor

shulcsm commented Jan 19, 2019

I think we agreed in #182 that strings shouldn't be accepted at all, why is this merged?

@btharper
Copy link
Contributor Author

This patch was intended to be the smallest change required to address a bug I found where large numbers passed as strings would be mangled by lack of granularity of floats. The effect on #182 was only found after finishing the patch.

To continue supporting the command line interface and keep backwards compatibility strings need to be accepted somewhere. Without getting into the merits of using strings, they are also currently used in some of the test cases as well.

No comments had been made against this pull request or inside of issue #182 when I referenced this PR. It also enforces the types on to_cardinal you had suggested and moves the string parsing into a single, intentional location.

As noted in my comment on #182, this doesn't handle normal thousands separators or locale aware parsing, only the simplest case presented in that issue.

@eortiz-tracktik eortiz-tracktik mentioned this pull request May 12, 2019
@btharper btharper deleted the strings branch October 6, 2019 04:38
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