Add DisableColors option (#764) #772
Conversation
|
|
|
@bruce-lindsay the code doesn't seem to compile... |
|
@davidfowl resolved. Sorry I made an error |
|
I have done everything from docker image microsoft/dotnet:2.1-sdk-stretch because the dotnet 2.1.4 doesn't tolerate the build target. as a result some blocks of code were windows only and could not be built or tested. I'm hoping I can lead on the review + CI to catch anything
|
| @@ -8,5 +8,6 @@ namespace Microsoft.Extensions.Logging.Console | |||
| public class ConsoleLoggerOptions | |||
| { | |||
| public bool IncludeScopes { get; set; } = false; | |||
| public bool DisableColors { get; set; } = false; | |||
There was a problem hiding this comment.
No need to set default "default values". I'd recommend removing the = false; from this line and the one above it.
There was a problem hiding this comment.
Addressed in 2bfbdfb
| private IExternalScopeProvider _scopeProvider; | ||
|
|
||
| public ConsoleLoggerProvider(Func<string, LogLevel, bool> filter, bool includeScopes) | ||
| public ConsoleLoggerProvider(Func<string, LogLevel, bool> filter, bool includeScopes, bool disableColors = false) |
There was a problem hiding this comment.
This looks like a breaking change. I recommend adding a 2nd constructor that has the extra parameter, and then chain the two constructors.
There was a problem hiding this comment.
Addressed in 2bfbdfb
| @@ -77,11 +86,14 @@ private void OnConfigurationReload(object state) | |||
| _settings = _settings.Reload(); | |||
|
|
|||
| _includeScopes = _settings?.IncludeScopes ?? false; | |||
| bool disableColors = _settings?.DisableColors ?? false; | |||
There was a problem hiding this comment.
addressed in 67c4ee5
|
Overall looks good to me, but I'm not a Logging expert, so I'll let someone from @muratg 's team review. |
|
Can you please add ConsoleLoggerOptions_IncludeScopes_IsReadFromLoggingConfiguration and ConsoleLoggerOptions_IncludeScopes_IsAppliedToLoggers test variations for console collors. |
|
@pakrym added requested test variations |
|
@muratg, this change involves a breaking change of rarely used |
|
I would add a 2nd interface that adds the new API. The bar for breaking changes is very high. |
|
@Eilon or we can just not support this switch for the old way of doing things ( |
|
I'm open to lots of options, but not ones involving breaking changes 😄 Your suggestion sounds fine to me. |
|
@bruce-lindsay I had to do couple changes to avoid breaking changes in legacy |
No description provided.