Added optional Culture parameter to NumberToWords #300

dmytro-gokun · 2014-06-18T17:20:23Z

No description provided.

…it culture)

…ecified culture

….Humanize

2) Explicitly registering DefaultFormatter's for the supported languages in FormatterRegistry's constructor

Conflicts: readme.md src/Humanizer.Tests/ApiApprover/PublicApiApprovalTest.approve_public_api.approved.txt src/Humanizer.Tests/DateHumanizeDefaultStrategyTests.cs src/Humanizer.Tests/TimeSpanHumanizeTests.cs src/Humanizer/Configuration/FormatterRegistry.cs src/Humanizer/Configuration/LocaliserRegistry.cs

mexx · 2014-06-18T20:48:45Z

readme.md

+Also, culture to use can be specified explicitly. If it is not, current thread's current UI culture is used. Here's an example:
+
+```C#
+11.ToWords(new CultureInfo("ru")) => "eleven"


The example is not correct, either it should be new CultureInfo("en") or the result "одинадцать"

Thx, i've fixed it.

С двумя "н"

@hazzik as i said, i've fixed sample. and there is no typo you mentioned. in fact, it only existed in @mexx 's comment :)

mexx · 2014-06-18T21:30:58Z

src/Humanizer/Localisation/NumberToWords/HebrewNumberToWordsConverter.cs

@@ -104,6 +107,11 @@ public override string Convert(int number, GrammaticalGender gender)
            return string.Join(" ", parts);
        }

+        public override string ConvertToOrdinal(int number, GrammaticalGender gender)
+        {
+            return number.ToString(_culture);


If we request 1234.ToWords(new CultureInfo("he-IL")) the _culture really used for formatting would be "he". Same applies to "pl" and "sl".
I think we should use the requested culture, maybe the formatting would deliver another result as the statically defined culture.

You are right. Let me explain my thoughts here:

We've discussed similar issue while i was working on DateTime.Humanize. It seems that @MehdiK decided that it's not important and merged DateTime.Humanize as it was.

The only reason we use "_culture" here is that HebrewNumberToWordsConverter misses ConvertToOrdinal implementation. Once that is implemented, issue will disappear.

For me it's something different as with DateTime.Humanize. In it we use the resources defined in our assembly and in here we utilize the standard functionality of the framework.
For DateTime.Humanize our implementation defined that the localization for region specific locale should be the same as for region unspecific one.
For our case we did not made such a decision, so we should use the specific locale.
Further it would preserve the current functionality, as currently number.ToString() is used, the specific culture if set as CurrentUICulture will be used.

@mexx I see your point. To work this around, we may pass the "culture" to the HebrewNumberToWordsConverter's constructor. But this means, that object have to be created each time number.ToWords() is called. Another option might be to pass "culture" to ConvertToOrdinal calls. Again, we discussed pros/cons in DateTime.Humanize()'s thread.

Personally, i do not think there is any real problem with creating new object on each call.
So, if @MehdiK agrees on this, i will change it accordingly.

I don't think either there is a problem of creating new object on each call, and it's already done for default localiser.

Ah, I see the problem.

Maybe instead of creating the converters on each call we can register HebrewNumberToWordsConverter with both "he" and "he-IL" passing the culture to the converter to be cached by the converter locally? That way the same class is used for both locales while the class still uses the provided culture.

Maybe instead of creating the converters on each call we can register HebrewNumberToWordsConverter with both "he" and "he-IL" passing the culture to the converter to be cached by the converter locally?

Sure, we can do this. But this does not solve issue for the default converter. Obviously, we can not pre-register an instance for each unsupported culture :).

Basically, if you remember my initial implementation of DateTime.Humanize ... i used to pass the culture object directly to the converter's methods. This solves issue with many-small-objects-have-to-be-created + we always can use actual culture instead of made-up one. But then people did not like it actively and we decided to pass the culture to the converter's constructor. So... what i think here... may be that was not that bad idea at all and we can do it that way? :)

I think we need to have a cache based on culture, then we can solve all the issues. Sure, it will require to write some code, but the proper working solution would be much better than incorrect one

@hazzik I do not think that cache is that good idea here, as we then have to take care of race conditions and stuff like that, which will make the library less usable with multithreaded server code (this is my primary area of interest and that's why i initially wanted to have this explicit culture support).

To be fair, i do not think that introducing locks just to avoid passing additional parameter makes any viable sense.

"may be that was not that bad idea at all and we can do it that way?"

I have to say, in the hindsight this is hard to argue with.

MehdiK · 2014-06-20T03:19:10Z

src/Humanizer/Localisation/NumberToWords/DefaultNumberToWordsConverter.cs

 {
-    internal class DefaultNumberToWordsConverter : INumberToWordsConverter
+    internal sealed class DefaultNumberToWordsConverter : GenderlessNumberToWordsConverter


Why did you make this class sealed?

P.S. In fact I've never found a legitimate case to create a sealed class.

hazzik · 2014-06-20T06:05:10Z

src/Humanizer/Localisation/NumberToWords/GenderlessNumberToWordsConverter.cs

+    {
+        /// <summary>
+        /// Converts the number to string using
+        /// </summary>


using what?

Added optional Culture parameter to NumberToWords

MehdiK · 2014-06-22T00:02:48Z

I am merging this as is. We can tackle the localisers in a separate PR.

MehdiK · 2014-06-22T00:11:54Z

For some reason I thought this was rebased and by merging it from GitHub I made a huge mess of the commit graph. Do'h. Sorry guys.

@dmitry-gokun please rebase your code before creating PRs. Thanks.

MehdiK · 2014-06-22T02:08:11Z

This is now released to NuGet as v1.27.0. Thanks for your contribution.

dmytro-gokun · 2014-06-22T03:43:08Z

For some reason I thought this was rebased and by merging it from GitHub I made a huge mess of the commit graph. Do'h. Sorry guys.

@dmitry-gokun please rebase your code before creating PRs. Thanks.

@MehdiK i'm sorry for this mess. I'm new to github, so i will try to be more careful :(

I am merging this as is. We can tackle the localisers in a separate PR.

So, if i understand you correctly, we should go with passing "culture" as a parameter to converter's methods (same for DateTime formatters)? Plz confirm and i will work on a PR for this.

This is now released to NuGet as v1.27.0. Thanks for your contribution.

Thanks 👍

MehdiK · 2014-06-22T12:16:50Z

@MehdiK i'm sorry for this mess. I'm new to github, so i will try to be more careful :(

No worries. That's alright. I should've checked before merging.

So, if i understand you correctly, we should go with passing "culture" as a parameter to converter's methods (same for DateTime formatters)? Plz confirm and i will work on a PR for this.

I'd rather not pass culture to every method tbh; but so far that seems like the cleanest option. I would like to hear what @mexx and @hazzik think about this too.

mexx · 2014-06-24T07:16:19Z

Actually I don't experience any issues with current design.
If we receive some issue complaining about memory consumption or speed then we can measure the impacts of the possible solutions, like introducing a cache in LocalizerRegistry or static registering the instances or passing the culture parameter or what else would come to our minds at this moment, and make the decision based on the results.

Dmitry Gokun added 13 commits May 28, 2014 22:37

Added option Culture parameter to DateTime.Humanize

530b2f5

Fixed formatting

adea66c

Reverted changes to DateHumanize (testing with both implicit & explic…

3dde23d

…it culture)

Added a test for getting culture-specific resource with explicitly sp…

abbc23e

…ecified culture

Fixed method's comment

0788a38

Only using optional "Culture" parameter for public methods

ed4121c

+ unit tests for DateTime.Humanize with explicit culure

1f8ad39

Updated readme to reflect new "culture" parameter for DateTime.Humanize

3bb7fec

Updated release_notes to reflect new "culture" parameter for DateTime…

464e7b4

….Humanize

Added optional Culture parameter to TimeSpan.Humanize

70f8f72

1) Made Date/Time formatters to own "culture";

c8bcd87

2) Explicitly registering DefaultFormatter's for the supported languages in FormatterRegistry's constructor

NumberToWords: added support for the explicit culture

2390f50

mexx reviewed Jun 18, 2014
View reviewed changes

Fixed NumberToWords sample

1e343d5

mexx reviewed Jun 18, 2014
View reviewed changes

Using real "Culture" object with NumberToWords converters

3d43f8e

MehdiK reviewed Jun 20, 2014
View reviewed changes

Small fixes during code review

4d4270c

hazzik reviewed Jun 20, 2014
View reviewed changes

Fixed comment

7ccfd20

MehdiK added a commit that referenced this pull request Jun 22, 2014

Merge pull request #300 from dmitry-gokun/master

08e76e9

Added optional Culture parameter to NumberToWords

MehdiK merged commit 08e76e9 into Humanizr:master Jun 22, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added optional Culture parameter to NumberToWords #300

Added optional Culture parameter to NumberToWords #300

dmytro-gokun commented Jun 18, 2014

mexx Jun 18, 2014

dmytro-gokun Jun 18, 2014

hazzik Jun 19, 2014

dmytro-gokun Jun 19, 2014

mexx Jun 18, 2014

dmytro-gokun Jun 18, 2014

mexx Jun 18, 2014

dmytro-gokun Jun 19, 2014

mexx Jun 19, 2014

MehdiK Jun 20, 2014

dmytro-gokun Jun 20, 2014

hazzik Jun 20, 2014

dmytro-gokun Jun 20, 2014

MehdiK Jun 21, 2014

MehdiK Jun 20, 2014

hazzik Jun 20, 2014

MehdiK commented Jun 22, 2014

MehdiK commented Jun 22, 2014

MehdiK commented Jun 22, 2014

dmytro-gokun commented Jun 22, 2014

MehdiK commented Jun 22, 2014

mexx commented Jun 24, 2014

Added optional Culture parameter to NumberToWords #300

Added optional Culture parameter to NumberToWords #300

Conversation

dmytro-gokun commented Jun 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MehdiK commented Jun 22, 2014

MehdiK commented Jun 22, 2014

MehdiK commented Jun 22, 2014

dmytro-gokun commented Jun 22, 2014

MehdiK commented Jun 22, 2014

mexx commented Jun 24, 2014