Added optional Culture parameter to DateTime.Humanize

[dmytro-gokun]

I've implemented optional Culture parameter for DateTime.Humanize + updated unit tests to check both explicit & implicit Culture cases.

Please check it, and if it's okay for you, i will proceed with adding Culture parameter to other Humanizer's methods.

[MehdiK]

Please format the method signature; i.e. precision to be moved to the end of the line

[dmytro-gokun]

Oops, sorry. I will fix it. But i just used included ReSharper config. May be it misses this rule and then it defaults to the rule defined on my PC's level? I'm not sure though.

[MehdiK]

Thanks for the effort. It looks great. Just a question and suggestion.

[dmytro-gokun]

Hi, Mehdi. So, what do we do from here? Do you want me to change something more for this pull request to be accepted?

[MehdiK]

Hey @dmitry-gokun,

Thanks for the effort. A few things are still needed for this PR:

• Fix the tests are commented.
• Implement explicit culture for all localised methods.
• Add your PR to the release notes with some description.
• Explain this in readme for each API outlining that cultures can be either injected or resolved through UI culture.

This is a relatively large change. So I am then going to ask @hazzik and @mexx and others to review your code too. In fact it would be great if more review happened at this stage before we go too far.

Thanks.

[dmytro-gokun]

Implement explicit culture for all localised methods.

I'm not quite sure what you mean here. Do you want PR contain ALL that changes to all Humanizer methods where that makes sense? To be fair, i would prefer to not go this way. It will be huge change, impossible to review etc. Why do not we go one feature at time? DateTime today, TimeSpan tomorrow and so on?

[MehdiK]

One feature at a time is my preferred option if you promise to commit to it ;-) Again that's because I don't want to be left with an inconsistent API and at the moment I don't have the bandwidth to do it myself.

[dmytro-gokun]

One feature at a time is my preferred option if you promise to commit to it ;-) Again that's because I don't want to be left with an inconsistent API and at the moment I don't have the bandwidth to do it myself.

Yes, i'll try to work on it near time. I can not promise to have it done in a week though, because i also have some day job and stuff like that :).

I've updated code as discussed. Not sure about unit tests though (in which file to put them and if what i have added is enough from your point of view or not). Please let me know.

[hazzik]

Shouldn't supplied/current culture go to a Formatter?

Something like this:
Register(culture => new DefaultFormatter(culture), localeCode);

[dmytro-gokun]

Sorry, i'm not sure if i completely get your comment. Can you elaborate please?

[hazzik]

We need to make an overload for Register method, which will accept Func<CultureInfo, IFormatter> to be able to get actual (supplied/current) culture instead of made-up culture used to register the formatter.

So for example if I have "ru-RU" as a current culture and configurator will select RussianFormatter because it matches "ru" culture, but the formatter will receive "ru" instead of "ru-RU".

Another application of this method - you don't need to register DefaultFormatter multiple times.

[dmytro-gokun]

This is not compatible with LocaliserRegistry implementation.

    public TLocaliser ResolveForCulture(CultureInfo culture)
    {
        culture = culture ?? CultureInfo.CurrentUICulture;

        Lazy<TLocaliser> factory;

        if (_localisers.TryGetValue(culture.Name, out factory))
            return factory.Value;

        if (_localisers.TryGetValue(culture.TwoLetterISOLanguageName, out factory))
            return factory.Value;

        return _defaultLocaliser;
    }

As you see, here we take a formatter from the cache. How would we supply it with culture?

I see your point here about passing actual culture to the formatter, but that means we can not derive FormatterRegistry from LocaliserRegistry anymore. Is this okay? Or should we change LocaliserRegistry to store a dictionary of constructors instead of dictionary of objects (and, thus, create a new formatter each time one is aquired? - is this efficient in general case?).

To be fair, all these considerations were the reason why i passed "culture" object to formatter's methods originally :).

[dmytro-gokun]

So, guys... Let me summarize what we have at the moment.
Basically, there are several ways to handle passing culture to a formatter:

1. An additional parameter to formatter's methods. Like i have done it originally.
   Pros: - the actual culture object is used, not some surrogate
   - no need to create a new formatter object on each call
   Cons: - the design is less "clean". Same culture object should be used when locating the formatter and when formatting value.

2. Use surrogate "culture" object in the formatter.
   Pros: - no need to create a new formatter object on each call, an object from the cache can be re-used.
   Cons: - surrogate "culture" object is used to locate string resources, not actual "culture" object passed to the Humanize() call. I'm not sure if this really poses any real-world problem. Do someone has any idea why this might be bad?

3. Pass actual "culture" object to the formatter's constructor.
   Pros: - actual culture is used
   Cons: - a new object has to be created each time. Sure, we can employ a kind of cache here, but it creates more problems then solves - particularly, we have to make sure the cache is thread-safe. I'm not sure if there will be any performance gain because of using such a cache.

Let's decide how we go there and i will implement it in a decided way.

[hazzik]

Just want to add cons to N2 (already discussed) - each time we add a new language with only resource - we need to add a registration for Formatter.

[hazzik]

Personally I'm ok to create a new formatter each time, as it is really-really cheap operation (we basically do not have any initialization logic). Probably it would be even cheaper than using Lazy<>. But one of valid concerns here that it will generate(?) a lot of garbage.

[dmytro-gokun]

I agree that creating a new object is probably cheaper then using Lazy<>. Especially, in a multithreaded application. But garbage might be a problem.

as it is really-really cheap operation (we basically do not have any initialization logic)

Well, this is true about current formatters. But, are you sure that always will be the case? Plus, this logic lives in LocaliserRegistry base class, which is also used by NumberToWordsConverterRegistry class and by some others. Are we sure that all those have very cheap constructors and will always have?

Actually, question here is if we really need all this complication and is not just passing "culture" parameter does the trick w/o introducing additional concerns.

About this:

Indeed, the formatter is retrieved with the notion of the culture already so it should know about the culture to use.
In other words it's an implementation detail of one particular implementation of the IFormatter interface and not a part of its input.

I'm not quite agree here. From my point of view "culture" is not a part of formatter's implementation. It's completely valid for a formatter to be able to serve different cultures. Look at CzechSlovakPolishFormatter, for example. Formatter just defines rules, which may apply to several languages. That's why i think it's completely valid to pass "culture" as a parameter to the formatter's methods.

[MehdiK]

I made a few changes on top of this, mostly to make the LocaliserRegistry and its usage easier, and created #293. Please check out and let me know what you think.

[dmytro-gokun]

It looks okay for me.

[mexx]

Looks good, I'm happy we could work it out without the culture parameter.

[MehdiK]

Cool. Closing this and merging the other guy now.

Thanks @dmitry-gokun for the awesome work and @hazzik and @mexx for reviewing and discussing the solution.

[MehdiK]

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