Merge pull request #2369 from JeanJoeris/jj/refactor-error-subsystem

Error Reporting Subsystem

Author: KathleenDollard

src/System.CommandLine.Tests/ParseErrorReportingTests.cs renamed to src/System.CommandLine.Subsystems.Tests/ErrorReportingFunctionalTests.cs

@@ -1,8 +1,10 @@
 // Copyright (c) .NET Foundation and contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+/*
 using System.CommandLine.Help;
 using System.CommandLine.Invocation;
+*/
 using System.IO;
 using FluentAssertions;
 using Xunit;
@@ -12,8 +14,10 @@
 
 namespace System.CommandLine.Tests;
 
-public class ParseErrorReportingTests
+public class ErrorReportingFunctionalTests
 {
+    // TODO: these tests depend on help output
+    /*
     [Fact] // https://github.com/dotnet/command-line-api/issues/817
     public void Parse_error_reporting_reports_error_when_help_is_used_and_required_subcommand_is_missing()
     {
@@ -126,4 +130,5 @@ public void When_no_help_option_is_present_then_help_is_not_shown_for_parse_erro
 
         output.ToString().Should().NotShowHelp();
     }
-}
+    */
+}

src/System.CommandLine.Subsystems.Tests/ErrorReportingSubsystemTests.cs

@@ -0,0 +1,84 @@
+// Copyright (c) .NET Foundation and contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using FluentAssertions;
+using Xunit;
+using System.CommandLine.Parsing;
+
+namespace System.CommandLine.Subsystems.Tests;
+
+public class ErrorReportingSubsystemTests
+{
+    [Fact]
+    public void Report_when_single_error_writes_to_console_hack()
+    {
+        var error = new ParseError("a sweet error message");
+        var errors = new List<ParseError> { error };
+        var errorSubsystem = new ErrorReportingSubsystem();
+        var consoleHack = new ConsoleHack().RedirectToBuffer(true);
+
+        errorSubsystem.Report(consoleHack, errors);
+
+        consoleHack.GetBuffer().Trim().Should().Be(error.Message);
+    }
+
+    [Fact]
+    public void Report_when_multiple_error_writes_to_console_hack()
+    {
+        var error = new ParseError("a sweet error message");
+        var anotherError = new ParseError("another sweet error message");
+        var errors = new List<ParseError> { error, anotherError };
+        var errorSubsystem = new ErrorReportingSubsystem();
+        var consoleHack = new ConsoleHack().RedirectToBuffer(true);
+
+        errorSubsystem.Report(consoleHack, errors);
+
+        consoleHack.GetBuffer().Trim().Should().Be($"{error.Message}{Environment.NewLine}{anotherError.Message}");
+    }
+
+    [Fact]
+    public void Report_when_no_errors_writes_nothing_to_console_hack()
+    {
+        var errors = new List<ParseError> { };
+        var errorSubsystem = new ErrorReportingSubsystem();
+        var consoleHack = new ConsoleHack().RedirectToBuffer(true);
+
+        errorSubsystem.Report(consoleHack, errors);
+
+        consoleHack.GetBuffer().Trim().Should().Be("");
+    }
+
+    [Theory]
+    [InlineData("-x")]
+    [InlineData("-non_existant_option")]
+    public void GetIsActivated_GivenInvalidInput_SubsystemIsActive(string input)
+    {
+        var rootCommand = new CliRootCommand {new CliOption<bool>("-v")};
+        var configuration = new CliConfiguration(rootCommand);
+        var errorSubsystem = new ErrorReportingSubsystem();
+        IReadOnlyList<string> args = [""];
+        Subsystem.Initialize(errorSubsystem, configuration, args);
+
+        var parseResult = CliParser.Parse(rootCommand, input, configuration);
+        var isActive = Subsystem.GetIsActivated(errorSubsystem, parseResult);
+
+        isActive.Should().BeTrue();
+    }
+
+    [Theory]
+    [InlineData("-v")]
+    [InlineData("")]
+    public void GetIsActivated_GivenValidInput_SubsystemShouldNotBeActive(string input)
+    {
+        var rootCommand = new CliRootCommand { new CliOption<bool>("-v") };
+        var configuration = new CliConfiguration(rootCommand);
+        var errorSubsystem = new ErrorReportingSubsystem();
+        IReadOnlyList<string> args = [""];
+        Subsystem.Initialize(errorSubsystem, configuration, args);
+
+        var parseResult = CliParser.Parse(rootCommand, input, configuration);
+        var isActive = Subsystem.GetIsActivated(errorSubsystem, parseResult);
+
+        isActive.Should().BeFalse();
+    }
+}

src/System.CommandLine.Subsystems.Tests/System.CommandLine.Subsystems.Tests.csproj

@@ -39,6 +39,8 @@
     <Compile Include="TestData.cs" />
     <Compile Include="VersionFunctionalTests.cs" />
     <Compile Include="VersionSubsystemTests.cs" />
+    <Compile Include="ErrorReportingFunctionalTests.cs" />
+    <Compile Include="ErrorReportingSubsystemTests.cs" />
   </ItemGroup>
 
   <ItemGroup>

src/System.CommandLine.Subsystems/ConsoleHack.cs

@@ -10,7 +10,7 @@ public class ConsoleHack
     private readonly StringBuilder buffer = new();
     private bool redirecting = false;
 
-    public void WriteLine(string text)
+    public void WriteLine(string text = "")
     {
         if (redirecting)
         {

src/System.CommandLine/ConsoleHelpers.cs renamed to src/System.CommandLine.Subsystems/ConsoleHelpers.cs

@@ -1,24 +1,49 @@
 // Copyright (c) .NET Foundation and contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+using System.CommandLine.Parsing;
 using System.CommandLine.Subsystems;
 using System.CommandLine.Subsystems.Annotations;
 
 namespace System.CommandLine;
 
+/// <summary>
+/// Subsystem for reporting errors
+/// </summary>
+/// <remarks>
+/// This class, including interface, is likey to change as powderhouse continues
+/// </remarks>
 public class ErrorReportingSubsystem : CliSubsystem
 {
     public ErrorReportingSubsystem(IAnnotationProvider? annotationProvider = null)
         : base(ErrorReportingAnnotations.Prefix, SubsystemKind.ErrorReporting, annotationProvider)
     { }
 
-    // TODO: Stash option rather than using string
     protected internal override bool GetIsActivated(ParseResult? parseResult)
         => parseResult is not null && parseResult.Errors.Any();
 
+    // TODO: properly test execute directly when parse result is usable in tests
     protected internal override CliExit Execute(PipelineContext pipelineContext)
     {
-        pipelineContext.ConsoleHack.WriteLine("You have errors!");
+        var _ = pipelineContext.ParseResult
+            ?? throw new ArgumentException("The parse result has not been set", nameof(pipelineContext));
+
+        Report(pipelineContext.ConsoleHack, pipelineContext.ParseResult.Errors);
+
         return CliExit.SuccessfullyHandled(pipelineContext.ParseResult);
     }
+
+    public void Report(ConsoleHack consoleHack, IReadOnlyList<ParseError> errors)
+    {
+        ConsoleHelpers.ResetTerminalForegroundColor();
+        ConsoleHelpers.SetTerminalForegroundRed();
+
+        foreach (var error in errors)
+        {
+            consoleHack.WriteLine(error.Message);
+        }
+        consoleHack.WriteLine();
+
+        ConsoleHelpers.ResetTerminalForegroundColor();
+    }
 }

src/System.CommandLine.Subsystems/ErrorReportingSubsystem.cs

@@ -10,7 +10,7 @@ public sealed class ParseError
     {
         // TODO: add position
         // TODO: reevaluate whether we should be exposing a SymbolResult here
-        internal ParseError(
+        public ParseError(
             string message, 
             SymbolResult? symbolResult = null)
         {