Refactored to support exception handling and less allocations#3
Refactored to support exception handling and less allocations#3OlegGavrilov wants to merge 1 commit into
Conversation
|
Hey, thanks so much for the contribution! I'm out of town right now, but I'll try and have a look over the changes here in the next week or so 👍 |
agc93
left a comment
agc93
left a comment
There was a problem hiding this comment.
Thanks again for the contribution. There's a lot of changes there and before I get too deep into it, I just want to ask:
Are you seeing any performance implications with the current implementation? I can see that a lot of the changes seem to be basically just to reduce the number of allocations so I want to know if the allocations are causing a problem you're facing or if it's optimising pre-emptively?
| public static class SpectreConsoleLoggingExtensions | ||
| { | ||
| public static ILoggingBuilder AddSpectreConsole(this ILoggingBuilder loggingBuilder, SpectreConsoleLoggerConfiguration config = null) | ||
| private static ILoggingBuilder AddSpectreConsole(this ILoggingBuilder loggingBuilder, SpectreConsoleLoggerConfiguration config = null) |
There was a problem hiding this comment.
Why make this private? That prevents consuming applications from using the extension method.
| } | ||
|
|
||
| public static ILoggingBuilder AddInlineSpectreConsole(this ILoggingBuilder loggingBuilder, SpectreConsoleLoggerConfiguration config = null) { | ||
| private static ILoggingBuilder AddInlineSpectreConsole(this ILoggingBuilder loggingBuilder, SpectreConsoleLoggerConfiguration config = null) { |
There was a problem hiding this comment.
Same as above, this prevents consuming apps from calling the extension.
| private const string Unknown = "[italic dim grey]unkn[/]: "; | ||
| private const string Trace = "[italic dim grey]trce[/]: "; | ||
| private const string Debug = "[dim grey]dbug[/]: "; | ||
| private const string Information = "[dim deepskyblue2]info[/]: "; | ||
| private const string Warning = "[bold orange3]warn[/]: "; | ||
| private const string Error = "[bold red]fail[/]: "; | ||
| private const string Critical = "[bold underline red on white]crit[/]: "; | ||
|
|
||
| private static string GetLevelMarkup(LogLevel level) | ||
| { | ||
| return level switch | ||
| { | ||
| LogLevel.Trace => "[italic dim grey]trce[/]: ", | ||
| LogLevel.Debug => "[dim grey]dbug[/]: ", | ||
| LogLevel.Information => "[dim deepskyblue2]info[/]: ", | ||
| LogLevel.Warning => "[bold orange3]warn[/]: ", | ||
| LogLevel.Error => "[bold red]fail[/]: ", | ||
| LogLevel.Critical => "[bold underline red on white]crit[/]: ", | ||
| _ => throw new ArgumentOutOfRangeException(nameof(level)) | ||
| LogLevel.Trace => Trace, | ||
| LogLevel.Debug => Debug, | ||
| LogLevel.Information => Information, | ||
| LogLevel.Warning => Warning, | ||
| LogLevel.Error => Error, | ||
| LogLevel.Critical => Critical, | ||
| _ => Unknown | ||
| }; | ||
| } |
There was a problem hiding this comment.
Just so I know, is this change intended to solve an actual problem you're facing?
I get that it's probably slightly more performant but it's also (in my view) dramatically less readable, so I'm not sure if you're actually seeing an issue here or if you're just trying to preemptively optimise?
2 participants
Hello! Hope it could be merged or maybe give some inspiration. Thanks!