search

jaraco / inflect

Correctly generate plurals, ordinals, indefinite articles; convert numbers to words
https://pypi.org/project/inflect
MIT License
957 stars 107 forks source link

number_to_words: support for custom comma-separators is broken/incomplete #176

Open jayaddison opened 1 year ago

jayaddison commented 1 year ago

Discovered while refactoring the number_to_words method, as part of reducing complexity of the package in #174:

Using a custom comma argument to number_to_words appears to be supported based on the docstrings, and that would match the corresponding documentation for the EN::Inflect CPAN module that this library is based on.

However: in practice it seems that providing a comma argument (other than the default ,) does not work as expected.

jayaddison commented 1 year ago

There could be a few ways in which this behaviour is odd/broken at the moment - here's one example (from exploratory extension of the existing test cases):

>>> import inflect
>>> e = inflect.engine()
>>> e.number_to_words(1000.3, threshold=500, comma="_") == "1_000.3"
False
>>> e.number_to_words(1000.3, threshold=500, comma="_")
'1,000.3'
jayaddison commented 1 year ago

(also to note: number_to_words isn't currently in use for any OpenCulinary/RecipeRadar-related software. The thinking behind reporting and addressing these kind of issues is that making the libraries we use better increases their usage and adoption rate by others, something that should hopefully result in further improvements in code quality over time)

jayaddison commented 1 year ago

Short-term, I think it may make sense to deprecate and/or remove the comma parameter to the number_to_words method. It's a breaking change to the interface, but the functionality hasn't been working correctly -- and the documented usage pattern can be achieved (I think) by the caller by performing a str.replace operation on the output (to replace , with their preferred substitution).

Arguably that imposes a potential unnecessary performance overhead -- but at the moment I don't think that the method is particularly highly performance-optimized. It should be possible to improve on that after some refactoring, and at that point it may make sense to reconsider and re-introduce the comma parameter.

jaraco commented 1 year ago

Thanks for the investigation. I'm totally fine with deprecating+removing the parameter.