CreateLogger<T>() should return ILogger<T>

[sunsided]

Since ILogger<T> already is an ILogger, shouldn't LoggerFactoryExtensions.CreateLogger<T>() return an ILogger<T> instead of an ILogger?

In order to automatically inject a logger, it needs to be of type ILogger<T>. If one needs to create an instance of a class with such a type in the constructor manually (e.g. in a unit test), ILoggerFactory is required (either way, since we can't construct Logger<T> without one).

However, no method or extension of that class is able to construct the required type ILogger<T> by itself. Instead, the type has to be created manually with new Logger<T>(factory) which is kind of awkward, especially since it internally calls factory.CreateLogger<T>() - that is, I'm now passing a logger factory to a logger I created in order to construct itself ...

The sad thing is that the neither of these perfectly valid looking options work

ILogger<Foo> foo = factory.CreateLogger<Foo>();
ILogger<Foo> bar = (ILogger<Foo>)factory.CreateLogger<Foo>();

where the second one explodes with a firework at runtime.

My suggestion would be to implement CreateLogger<T>() as

public static ILogger<T> CreateLogger<T>(this ILoggerFactory factory)
{
    if (factory == null)
    {
        throw new ArgumentNullException(nameof(factory));
    }

    return new Logger<T>(factory);
}

and the constructor of Logger<T> as

public Logger(ILoggerFactory factory)
{
    _logger = factory.CreateLogger(TypeNameHelper.GetTypeDisplayName(typeof(T), fullName: true));
}

instead.

[muratg]

This makes sense. @davidfowl what do you think?

@sunsided Would you be interested in sending a PR?

[sunsided]

@muratg I am, will do. 👍

[muratg]

@sunsided Thanks!

[Eilon]

@lodejard as well. I have a feeling there was some reason for this but I don't remember what it is. Does it have to do with some casting issues that happen in consumers of this or something?

[muratg]

@Eilon ILogger<T> implements ILogger so this shouldn't require any casting from the consumer side AFAIK.

[Eilon]

Yeah but I feel like there was a reverse substitutability problem or something. Or I could be completely mistaken. Like now that you force taking an ILogger<T> you can't take an ILogger anymore and there might be times where only an ILogger is available. The reality is that ILogger<T> doesn't do anything - it's just a fancy trick to get a type name. It has no meaning of its own. In theory the type shouldn't even exist. But maybe this is fine...

[muratg]

@lodejard Any thoughts on this one?

[muratg]

@rynowak Do you remember if we had a reason for the current design as @Eilon alludes to?

[rynowak]

I don't recall any of that. I wasn't involved with the design of ILogger<T>

[lodejard]

There's no reason, it's just that ILogger CreateLogger<T> was always present, and ILogger<T> was added much later as an IoC trick. We can change it to ILogger<T> CreateLogger<T>() - it'll be returning a wrapper but that should be fine.

[muratg]

Thanks! @kichalla, could you review and take the PR in?

@sunsided Thanks for the contribution.

[kichalla]

d679c78