Refactored to support exception handling and less allocations

src/Spectre.Console.Extensions.Logging/BaseSpectreLogger.cs

@@ -0,0 +1,87 @@
+using System;
+using Microsoft.Extensions.Logging;
+
+namespace Spectre.Console.Extensions.Logging
+{
+    public abstract class BaseSpectreLogger : ILogger
+    {
+        /* 
+        / Because of the context this implementation originated in (a CLI tool)
+        / this logger doesn't actually *use* the category name anywhere.
+        */
+        private protected readonly string Name;
+        private protected readonly SpectreConsoleLoggerConfiguration Config;
+        private readonly IAnsiConsole _console;
+
+        protected BaseSpectreLogger(string name, SpectreConsoleLoggerConfiguration config)
+        {
+            Name = name;
+            Config = config;
+
+            var settings = config.ConsoleSettings ?? new AnsiConsoleSettings
+            {
+                Ansi = AnsiSupport.Detect,
+                ColorSystem = ColorSystemSupport.Detect
+            };
+            _console = AnsiConsole.Create(settings);
+            config.ConsoleConfiguration?.Invoke(_console);
+        }
+
+        protected abstract string RenderLogMessage<TState>(LogLevel logLevel, EventId eventId, TState state,
+            Exception exception, Func<TState, Exception, string> formatter);
+
+        public IDisposable BeginScope<TState>(TState state)
+        {
+            return null;
+        }
+
+        public bool IsEnabled(LogLevel logLevel)
+        {
+            return logLevel >= Config.LogLevel;
+        }
+
+        public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception,
+            Func<TState, Exception, string> formatter)
+        {
+            if (!IsEnabled(logLevel)) return;
+
+            if (Config.EventId == 0 || Config.EventId == eventId.Id)
+            {
+                var formattedLine = RenderLogMessage(logLevel, eventId, state, exception, formatter);
+
+                switch (Config.InvalidMarkup)
+                {
+                    case InvalidMarkupHandling.WriteAsIs:
+                        try
+                        {
+                            _console.MarkupLine(formattedLine);
+                        }
+                        catch (Exception)
+                        {
+                            _console.WriteLine(formattedLine);
+                        }
+
+                        break;
+                    case InvalidMarkupHandling.WriteAsIsAndException:
+                        try
+                        {
+                            _console.MarkupLine(formattedLine);
+                        }
+                        catch (InvalidOperationException ex)
+                        {
+                            _console.WriteLine(formattedLine);
+                            _console.WriteLine("Failed to render previous line with Spectre, details:");
+                            _console.WriteLine(ex.ToString());
+                        }
+
+                        break;
+                    case InvalidMarkupHandling.Throw:
+                        _console.MarkupLine(formattedLine);
+                        break;
+                    default:
+                        throw new ArgumentOutOfRangeException(nameof(Config.InvalidMarkup));
+                }
+            }
+        }
+    }
+}

src/Spectre.Console.Extensions.Logging/InvalidMarkupHandling.cs

@@ -0,0 +1,9 @@
+namespace Spectre.Console.Extensions.Logging
+{
+    public enum InvalidMarkupHandling
+    {
+        WriteAsIs,
+        WriteAsIsAndException,
+        Throw,
+    }
+}

src/Spectre.Console.Extensions.Logging/SpectreConsoleLogger.cs

@@ -1,65 +1,68 @@
 using System;
+using System.Text;
 using Microsoft.Extensions.Logging;
 
 namespace Spectre.Console.Extensions.Logging
 {
-    public class SpectreConsoleLogger : ILogger
+    public class SpectreConsoleLogger : BaseSpectreLogger
     {
         /* 
         / Because of the context this implementation originated in (a CLI tool)
         / this logger doesn't actually *use* the category name anywhere.
         */
-        private readonly string _name;
-        private readonly SpectreConsoleLoggerConfiguration _config;
-        private readonly IAnsiConsole _console;
+        public SpectreConsoleLogger(string name, SpectreConsoleLoggerConfiguration config) : base(name, config) { }
+
+        protected override string RenderLogMessage<TState>(LogLevel logLevel, EventId eventId, TState state,
+            Exception exception, Func<TState, Exception, string> formatter)
+        {
+            var formattedLine = formatter(state, exception);
 
-        public SpectreConsoleLogger(string name, SpectreConsoleLoggerConfiguration config) {
-            _name = name;
-            _config = config;
+            var sb = new StringBuilder();
 
-            var settings = config.ConsoleSettings ?? new AnsiConsoleSettings {
-                Ansi = AnsiSupport.Detect,
-                ColorSystem = ColorSystemSupport.Detect
-            };
-            _console = AnsiConsole.Create(settings);
-            if (config.ConsoleConfiguration != null) {
-                config.ConsoleConfiguration.Invoke(_console);
+            if (Config.IncludePrefix)
+            {
+                var prefix = GetLevelMarkup(logLevel);
+
+                sb.Append(prefix);
             }
-        }
-        public IDisposable BeginScope<TState>(TState state) {
-            return null;
-        }
 
-        public bool IsEnabled(LogLevel logLevel) {
-            return logLevel >= _config.LogLevel;
-        }
+            sb.Append(Name);
 
-        public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func<TState, Exception, string> formatter) {
-            if (!IsEnabled(logLevel)) {
-                return;
-            }
-            if (_config.EventId == 0 || _config.EventId == eventId.Id) {
-                var prefix = _config.IncludePrefix
-                    ? GetLevelMarkup(logLevel)
-                    : string.Empty;
-                var categoryStr = _config.IncludeEventId
-                    ? _name + $"[grey][[{eventId.Id}]][/]"
-                    : _name;
-                _console.MarkupLine(prefix + categoryStr);
-                _console.MarkupLine(string.Empty.PadRight(6) + formatter(state, exception));
+            if (Config.IncludeEventId)
+            {
+                sb.Append("[grey][[");
+                sb.Append(eventId.Id);
+                sb.Append("]][/]");
             }
+
+            sb.AppendLine();
+
+            sb.Append("      ");
+            sb.Append(formattedLine);
+
+            return sb.ToString();
         }
-        private string GetLevelMarkup(LogLevel level) {
+
+        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
             };
         }
     }
-}
+}

src/Spectre.Console.Extensions.Logging/SpectreConsoleLoggerConfiguration.cs

@@ -11,5 +11,6 @@ public class SpectreConsoleLoggerConfiguration
         public AnsiConsoleSettings ConsoleSettings {get;set;} = null;
         public bool IncludePrefix {get;set;} = true;
         public bool IncludeEventId {get;set;} = false;
+        public InvalidMarkupHandling InvalidMarkup { get; set; } = InvalidMarkupHandling.WriteAsIs;
     }
 }

src/Spectre.Console.Extensions.Logging/SpectreConsoleLoggingExtensions.cs

@@ -1,12 +1,11 @@
 using System;
-using Microsoft.Extensions.Logging;
 using Spectre.Console.Extensions.Logging;
 
 namespace Microsoft.Extensions.Logging
 {
     public static class SpectreConsoleLoggingExtensions
     {
-        public static ILoggingBuilder AddSpectreConsole(this ILoggingBuilder loggingBuilder, SpectreConsoleLoggerConfiguration config = null)
+        private static ILoggingBuilder AddSpectreConsole(this ILoggingBuilder loggingBuilder, SpectreConsoleLoggerConfiguration config = null)
         {
             loggingBuilder.AddProvider(new SpectreConsoleLoggerProvider(config ?? new SpectreConsoleLoggerConfiguration()));
             return loggingBuilder;
@@ -19,7 +18,7 @@ public static ILoggingBuilder AddSpectreConsole(this ILoggingBuilder loggingBuil
             return loggingBuilder.AddSpectreConsole(config);
         }
 
-        public static ILoggingBuilder AddInlineSpectreConsole(this ILoggingBuilder loggingBuilder, SpectreConsoleLoggerConfiguration config = null) {
+        private static ILoggingBuilder AddInlineSpectreConsole(this ILoggingBuilder loggingBuilder, SpectreConsoleLoggerConfiguration config = null) {
             loggingBuilder.AddProvider(new SpectreInlineLoggerProvider(config ?? new SpectreConsoleLoggerConfiguration()));
             return loggingBuilder;
         }

src/Spectre.Console.Extensions.Logging/SpectreInlineLogger.cs

@@ -1,72 +1,57 @@
 using System;
+using System.Text;
 using Microsoft.Extensions.Logging;
 
 namespace Spectre.Console.Extensions.Logging
 {
-    public class SpectreInlineLogger : Microsoft.Extensions.Logging.ILogger {
-        /* 
-        / Because of the context this implementation originated in (a CLI tool)
-        / this logger doesn't actually *use* the category name anywhere.
-        */
-        private readonly string _name;
-        private readonly SpectreConsoleLoggerConfiguration _config;
-        private readonly IAnsiConsole _console;
-
-        public SpectreInlineLogger(string name, SpectreConsoleLoggerConfiguration config) {
-            _name = name;
-            _config = config;
-
-            var settings = config.ConsoleSettings ?? new AnsiConsoleSettings {
-                Ansi = AnsiSupport.Detect,
-                ColorSystem = ColorSystemSupport.Detect
-            };
-            _console = AnsiConsole.Create(settings);
-            if (config.ConsoleConfiguration != null) {
-                config.ConsoleConfiguration.Invoke(_console);
+    public class SpectreInlineLogger : BaseSpectreLogger{
+
+        public SpectreInlineLogger(string name, SpectreConsoleLoggerConfiguration config) : base(name, config) { }
+
+        protected override string RenderLogMessage<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func<TState, Exception, string> formatter)
+        {
+            var formattedLine = formatter(state, exception);
+
+            var sb = new StringBuilder();
+            if (Config.IncludePrefix)
+            {
+                var prefix = GetLevelMarkup(logLevel);
+                sb.Append(prefix);
             }
-        }
-        public IDisposable BeginScope<TState>(TState state) {
-            return null;
-        }
-
-        public bool IsEnabled(LogLevel logLevel) {
-            return logLevel >= _config.LogLevel;
-        }
-
-        public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func<TState, Exception, string> formatter) {
-            if (!IsEnabled(logLevel)) {
-                return;
+
+            if (Config.IncludeEventId)
+            {
+                var eventIdMarkup = GetEventIdMarkup(eventId);
+                sb.Append(eventIdMarkup);
             }
-            if (_config.EventId == 0 || _config.EventId == eventId.Id) {
-                var prefix = _config.IncludePrefix
-                    ? GetLevelMarkup(logLevel)
-                    : string.Empty;
-                var eventIdStr = _config.IncludeEventId
-                    ? GetEventIdMarkup(eventId) + " "
-                    : string.Empty;
 
-                _console.MarkupLine(prefix + eventIdStr + formatter(state, exception));
-            }
-        }
+            sb.Append(formattedLine);
 
-        private string GetEventIdMarkup(EventId evId) {
-            if (evId.Id == 0) {
-                return string.Empty.PadRight(7);
-            }
-            var eventId = string.Format("{0,4:####}", evId.Id);
-            return $"[dim grey][[{eventId}][/] ";
+            return sb.ToString();
         }
-
-        private string GetLevelMarkup(LogLevel level) {
-            return level switch {
-                LogLevel.Critical => "[bold underline white on red]| CRIT|:[/] ",
-                LogLevel.Error => "[bold red]|ERROR|:[/] ",
-                LogLevel.Warning => "[bold orange3]| WARN|:[/] ",
-                LogLevel.Information => "[bold dim]| INFO|:[/] ",
-                LogLevel.Debug => "[dim]|DEBUG|:[/] ",
-                LogLevel.Trace => "[dim grey]|TRACE|:[/] ",
-                _ => "| UNKN|: "
+
+        private static string GetEventIdMarkup(EventId eventId) => eventId.Id == 0 ? EmptyEventIdMarkup : string.Format(EventIdMarkup, eventId.Id);
+
+        private const string EmptyEventIdMarkup = "       ";
+        private const string EventIdMarkup = "[dim grey][[{0,4:####}]][/] ";
+
+        private const string Unknown = "| UNKN|: ";
+        private const string Trace = "[dim grey]|TRACE|:[/] ";
+        private const string Debug = "[dim]|DEBUG|:[/] ";
+        private const string Information = "[bold dim]| INFO|:[/] ";
+        private const string Warning = "[bold orange3]| WARN|:[/] ";
+        private const string Error = "[bold red]|ERROR|:[/] ";
+        private const string Critical = "[bold underline white on red]| CRIT|:[/] ";
+
+        private static string GetLevelMarkup(LogLevel level) =>
+            level switch {
+                LogLevel.Critical => Critical,
+                LogLevel.Error => Error,
+                LogLevel.Warning => Warning,
+                LogLevel.Information => Information,
+                LogLevel.Debug => Debug,
+                LogLevel.Trace => Trace,
+                _ => Unknown
             };
-        }
     }
 }