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

Fixed "1 million" number to words in Spanish #235

Merged
merged 2 commits into from
Apr 19, 2014
Merged

Fixed "1 million" number to words in Spanish #235

merged 2 commits into from
Apr 19, 2014

Conversation

kblok
Copy link
Contributor

@kblok kblok commented Apr 19, 2014

The "1 million" in letters were wrong in Spanish. It is "un millón" not just simply "millón"
Source: http://es.wikipedia.org/wiki/Anexo:Nombres_de_los_n%C3%BAmeros_en_espa%C3%B1ol

With this PR I finish my review on NumberToLetters in Spanish.

@MehdiK @thunsaker Maybe I'll sound quite purist but we are not validating if we are processing correctly the conversion. For instance: In English the output would be wrong for numbers above (or equal than) a trillion and in Spanish the output would be wrong with numbers above (or equal than) a billion.
If we don't want to waste our time coding for big (and unusual) numbers, we should throw and exception at least.

@thunsaker
Copy link
Contributor

I originally had it that way, but modified it. So I was a little too aggressive in removing "un" from numbers as per the recommendation of @bangoker ? see comment

Thanks for the update!

@kblok
Copy link
Contributor Author

kblok commented Apr 19, 2014

I think @bangoker was talking about "un mil" and "un mil millones", which correction es right, but the "un" is necessary with "millón", you say "un millón" not just "millón" and you say "mil" not "un mil".
"Un mil" it is said when someone in TV are telling the lottery number, so they want to be clear and they kind of "spell" the number.

MehdiK added a commit that referenced this pull request Apr 19, 2014
Fixed "1 million" number to words in Spanish
@MehdiK MehdiK merged commit 9d32126 into Humanizr:master Apr 19, 2014
@MehdiK
Copy link
Member

MehdiK commented Apr 19, 2014

@kblok thanks for the correction.

WRT NumberToWords for large numbers, the method accepts int as parameter (maxing at 2 billions). In English we're verifying up to 1,234,567,890. I cannot be certain about this; but I think we're safe to assume it supports the whole range. Different locale contributors have taken liberty in how far up the range their implementation supports. Admittedly I've been quite forgiving of the algorithms and support of different localisations mainly because verifying them requires deep knowledge of the language, which I don't have.

Obviously we could enforce the range on the tests; but at times, in this and other OSS projects, when I push the contributors a bit far they abandon the PR and even the project altogether. So I thought I'd let the localisation flow through however incomplete they might be and eventually the community fixes the incomplete/buggy implementations. Fortunately, so far it's worked :)

@kblok
Copy link
Contributor Author

kblok commented Apr 19, 2014

When I read the link @thunsaker post, I figured out that, as you said, using int32 datatypes "protect" us from those unsupported big numbers.
After that I double checked and the Spanish language implementation covers all the range of an int32 datataype

@MehdiK
Copy link
Member

MehdiK commented Apr 21, 2014

Thanks for the contribution. This is now available on NuGet as v1.24.1.

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.

3 participants