Ability to override methods in JsonStringLocalizer and JsonStringLocalizerFactory

[thomkle]

It would be nice to be able to override methods in in JsonStringLocalizer and JsonStringLocalizerFactory by making them virtual and at least using the protected modifier. E.g. if I'd like to change the functionality on how the cache is build, there is currently no way of overriding just that method (BuildResourcesCache), I'd have to make a new localizer by copying the content from JsonStringLocalizer.

Stumbled upon this when wanting to try to implement a temporary workaround to support the #39 functionality for the current release.

[hishamco]

I'd like to change the functionality on how the cache is build, there is currently no way of overriding just that method (BuildResourcesCache),

This will may assist to fix #34

Could you please let me know what's the suggested changes that you like to made?

[thomkle]

For the JsonStringLocalizerFactory, just simple things to make extensions easier to implement:

• make _resourcesRelativePath protected
• make _resourcesType protected
• make _loggerFactory protected
• make Create(Type resourceSource) virtual
• make Create(string baseName, string location) virtual
• make CreateJsonStringLocalizer return the generic IStringLocalizer (otherwise if we wanted to override this to return an extension of JsonStringLocalizer, it would make it hard to achieve)
• make GetResourcePath protected and virtual

For the JsonStringLocalizer do the following:

• make _resourcesCache protected
• make _resourcesPath protected
• make _resourceName protected
• make _logger protected
• make _searchedLocation protected
• make GetAllStrings(bool includeParentCultures, CultureInfo culture) virtual
• make GetStringSafely virtual
• move the building of the cache and getting the value logic in GetStringSafely to its own virtual protected method (e.g. BuildCacheAndGetValue) or, like I did in PR Added functionality to support a 'root' fallback resource. #39 (GetStringSafely(string name, CultureInfo culture)), but also adding virtual
• make GetAllStringsFromCultureHierarchy protected and virtual
• make GetAllResourceStrings protected and virtual
• make BuildResourcesCache protected and virtual and/or move the getting of the resource file name logic to its own protected virtual method (e.g. GetResourceFileName)

I think these changes would make the library very flexible to extensions and customized changed for the users of the library. What do you think? Too much exposure?

When it comes to adding support for changeable caching mechanism, what about adding a protected virtual method called GetOrAddCache that has the same input parameters as the ConcurrentDictionary.GetOrAdd (TKey key, Func<TKey, TValue>, or in this case string, Func<string, IEnumerable<KeyValuePair<string, string>>>)? Then the user of the library is free to override this method and provide his own caching mechanism.

Edit: forgot to add the Create-methods in the list.

[hishamco]

FYI some of the method couldn't be override able, let us think from many perspectives and choose the proper methods to make them virtual with Why question

Thanks for your suggestion

[thomkle]

I see, I can probably note the most important issues I have with extending the library at the moment:

• If I want to extend the JsonStringLocalizer I need to also extend the JsonStringLocalizerFactory to override the CreateJsonStringLocalizer to return the extended type, however, the type of this method is a static JsonStringLocalizer. If we set the return type to IStringLocalizer, this can be overridden to return the new extended type. Also, the PR that adds _defaultCulture variable needs to either be an input parameter in the method or a non-private variable (since we need access to it when overriding the CreateJsonStringLocalizer).
• To be able to do override any needed changes to an extension of the JsonStringLocalizer, I need to be able to at least change how the fallback is working (being able to change the resource file name logic), which involves the GetStringSafely- and BuildResourcesCache-methods.

I tried extending to implement my required fall back scenario, but was not able to (as I currently can't override any of the methods) and ended up having to copy the two classes and change them my self and add the line services.TryAddSingleton<IStringLocalizerFactory, JsonStringLocalizerFactoryExtended>(); in front of the services.AddJsonLocalization(); initialization. The services.TryAddSingleton<IStringLocalizerFactory, JsonStringLocalizerFactoryExtended>(); could be improved also by adding a generic extension method (e.g. services.AddJsonLocalization<JsonStringLocalizerFactoryExtended>();) maybe?

[hishamco]

Do you have a repo for your fallback scenario? or I will provide a sample for such thing an probably I unit tests may be useful to ensure that nothing is broken

[thomkle]

No repo, sorry. But the only change I basically need after your PR #41 is just removing the line && requestedCulture.Name == _defaultCulture-part on line 131. That would make the fallback happen for all cultures like I need. A sample of extending the JsonStringLocalizer would be greatly appreciated!

[hishamco]

@thomkle I just created a sample in https://github.com/hishamco/My.Extensions.Localization.Json/tree/custom-localizer with minimal modification in the original JsonStringLocalizer, BTW you could notice that Yes will have the same value Oui in all the cultures, which is the thing that I told you about before, so again this should respect the default fallback culture

Hope this fix your issue

[hishamco]

@thomkle I'd like to let you know I'm working on this now, but I need to from you what kind of extensibility do you need, I don't think adding protected virtual to every method is a correct choice

[hishamco]

Add this for now 0a48303

I will try to work on improving the BuildResourceCache hopefully I will add an extensibility point there ...