-
Notifications
You must be signed in to change notification settings - Fork 839
Add LoggerFactory.Create method #640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
||
<Reference Include="Microsoft.Extensions.Configuration.Binder" /> | ||
<Reference Include="Microsoft.Extensions.DependencyInjection.Abstractions" /> | ||
<Reference Include="Microsoft.Extensions.DependencyInjection" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is slightly unfortunate. But not the end of the world I guess.
public bool ShouldDispose; | ||
} | ||
|
||
private class DisposingLoggerFactory: ILoggerFactory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that we need to hold onto the container means all of the sample code is sorta busted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Busted how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you’re saying we don’t need to dispose the service provider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this wrapper exist to begin with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To dispose ILoggerProviders created by DI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn’t disposing the factory do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see it wouldn’t. That makes the current usage without this API leaky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't dispose the service provider - yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m taking about examples that you would use today (even in our samples)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a sample fix.
|
||
<Reference Include="Microsoft.Extensions.Configuration.Binder" /> | ||
<Reference Include="Microsoft.Extensions.DependencyInjection.Abstractions" /> | ||
<Reference Include="Microsoft.Extensions.DependencyInjection" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this change caused these failures in my PR build. It actually showed up in your PR check too, but as a warning instead of an error.
@natemcmaster 1. Is the proper solution here (assuming it is an approved breaking change) to edit this line? 2. Should this kind of change be a warning or an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ryanbrandenburg does your PR invoke build with --warnaserror
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what Arcade defaults to. We can certainly override that anywhere (or everywhere), but I thought I'd ask since this seems like the kind of thing we might want to fail a build (even if it's just so we can document that this was an intended break).
Commit migrated from dotnet/extensions@1414cac
Commit migrated from dotnet/extensions@1414cac
Commit migrated from dotnet/extensions@1414cac
Commit migrated from dotnet/extensions@1414cac
Adds the
LoggerFactory.Create
that simplifies LoggerFactory creation in cases where dependency injection is not used in the application.So the pattern becomes:
Fixes: #615