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

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

Open
jayaddison opened this issue Feb 13, 2023 · 4 comments

Comments

@jayaddison
Copy link
Contributor

jayaddison commented Feb 13, 2023

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
Copy link
Contributor Author

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 jayaddison changed the title num_to_words: support for custom comma-separators is broken/incomplete number_to_words: support for custom comma-separators is broken/incomplete Feb 13, 2023
@jayaddison
Copy link
Contributor Author

(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
Copy link
Contributor Author

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
Copy link
Owner

jaraco commented Apr 6, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants