search

aspnet / MetaPackages

[Archived] NuGet meta packages. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
211 stars 109 forks source link

Wire up the IncludeScopes configuration option, closes #204 #210

Closed NinoFloris closed 7 years ago

NinoFloris commented 7 years ago

This option should affect the ConsoleLogger and isn't today

dnfclas commented 7 years ago

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request. Thanks, .NET Foundation Pull Request Bot

Eilon commented 7 years ago

@muratg - who should review this?

muratg commented 7 years ago

Assigning to @JunTaoLuo for review.

JunTaoLuo commented 7 years ago

This looks odd. We pass in the configuration to the ILoggerBuilder but it does not flow the IncludeScopes configuration to the ConsoleLogger. I feel that the configuration of our loggers needs an update instead of this change. cc @BrennanConroy @pakrym thoughts?

pakrym commented 7 years ago

This is caused by bigger issue and this is not the right solution.

Issue is that we need a general way to wire logger providers ConfigureOptions to IConfiguration instances assigned to logging configuration.

Solution requires some design.

pakrym commented 7 years ago

Filed: https://github.com/aspnet/Logging/issues/688

NinoFloris commented 7 years ago

Cool don't mind how it's fixed, saw that you already had some issues open around flowing IncludeScopes config. When filed I hoped this quick fix could still make RTM but alas. Thanks for picking it up