Extensions: Should IStringLocalizer be covariant?

[kevinchalet]

Like ILogger<T>, IOptions<T> or IOptionsMonitor<T>, should IStringLocalizer<T> be covariant? This would allow this kind of pattern:

public class Base
{
    public Base(IStringLocalizer<Base> localizer) { }
}

public class Derived : Base
{
    public Base(IStringLocalizer<Derived> localizer)
        : base(localizer) { }
}

[kevinchalet]

@Eilon did you guys have a chance to discuss this change? If it's accepted, I'd love to submit a PR as I plan to use the localization stack in my OpenIddict project.

[Eilon]

Tagging @davidfowl and @mkArtakMSFT

I assume it's not a breaking change to add out?

I do like this change, and it's certainly nice in logging.

[kevinchalet]

I assume it's not a breaking change to add out?

Yep, it's not a breaking change (I just tested to confirm it).

[Eilon]

@mkArtakMSFT / @davidfowl / @ryanbrandenburg - any thoughts on this? This sounds good to me.

(BTW Fowler has no power, and RyanB on vaca, so might be slow response right now.)

[davidfowl]

I'm in a hotel now 😄, seems fine, I'm not sure what the use case is though. Do you have something concrete @PinpointTownes ?

[kevinchalet]

Do you have something concrete @PinpointTownes ?

Yeah, the main scenario I have in mind is to allow an application to override the resources location when subclassing a class from a class library that comes with .resx files and uses [assembly: ResourceLocation("...")].

// Defined in a class library.
public class Base
{
    // Uses the resources defined in the class library (ClassLibrary.Resources.Base).
    public Base(IStringLocalizer<Base> localizer) { }
}

// Subclassed in application code.
public class Derived : Base
{
    // Uses the resources defined in the application (App.Resources.Derived).
    // Both this class and the base one will use the application strings.
    public Base(IStringLocalizer<Derived> localizer)
        : base(localizer) { }
}

It's exactly like how options and generic loggers work:

public class Base
{
    public Base(IOptions<Base> options) { }
}

public class Derived : Base
{
    // Resolve specialized Derived options and inject them
    // into the base class as IOptions<Base>.
    public Base(IOptions<Derived> options)
        : base(options) { }
}

It's something I use in OpenIddict: I have a OpenIddictServerOptions class that inherits from OpenIdConnectServerOptions (from ASOS) and defines additional options. Wherever IOptionsMonitor<OpenIdConnectServerOptions> is used, I simply flow IOptionsMonitor<OpenIddictServerOptions> (e.g https://github.com/openiddict/openiddict-core/blob/dev/src/OpenIddict.Server/OpenIddictServerHandler.cs#L25).

[hishamco]

LGTM /cc @rynowak @ryanbrandenburg @pranavkm

[kevinchalet]

Excellent, I'll send a PR soon, then 😄

[ryanbrandenburg]

Thinking through this now I wonder if the code isn't correct as it is today. You gave some examples of C# code, but lets consider some resx KVPs that might be in those classes.

|Key|Base value|Derived value|
|----|-----------|---------------|
|Greeting|A basic greeting to you!|A specific greeting to you!|

So now IStringLocalizer<Base> base could return either a basic greeting or a specific greeting, which seems wrong to me. @davidfowl thoughts? I feel like making it covariant only makes sense if we actually walk the class inheritance tree as part of class localization, which we do not today (and would be a pretty complicated system if we did).

[ryanbrandenburg]

Talked it over with @danroth27 and decided that what I described was not only fine but exactly what was being asked for. I'll look into the PR.

[ryanbrandenburg]

Thanks @PinpointTownes!

[kevinchalet]

@ryanbrandenburg thanks for merging it! 👏