-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,7 @@ | |
<Compile Include="../../shared/*.cs" /> | ||
|
||
<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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. @ryanbrandenburg does your PR invoke build with There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). |
||
<Reference Include="Microsoft.Extensions.Logging.Abstractions" /> | ||
<Reference Include="Microsoft.Extensions.Options" /> | ||
</ItemGroup> | ||
|
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.
Uh oh!
There was an error while loading. Please reload this page.
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.