From 4443107eff529ffebe06ddbb0eb11e28782e5888 Mon Sep 17 00:00:00 2001 From: KathleenDollard Date: Thu, 23 May 2024 09:47:55 -0400 Subject: [PATCH 1/2] Remove CliExit and add SetSuccess/NotRun The SetSuccess and NotRun methods are intended to abstract what those conditions mean from the actual properties that are set, probably making them private after discussio. I have more confidence in SetSuccess than NotRun --- .../AlternateSubsystems.cs | 17 ++++----- .../PipelineTests.cs | 10 +++--- .../VersionFunctionalTests.cs | 2 +- src/System.CommandLine.Subsystems/CliExit.cs | 35 ------------------- .../CompletionSubsystem.cs | 5 +-- .../Directives/DiagramSubsystem.cs | 5 +-- .../ErrorReportingSubsystem.cs | 5 +-- .../HelpSubsystem.cs | 5 +-- src/System.CommandLine.Subsystems/Pipeline.cs | 20 +++++------ .../Subsystems/CliSubsystem.cs | 13 ++++--- .../Subsystems/PipelineResult.cs | 12 +++++++ .../Subsystems/Subsystem.cs | 8 ++--- .../ValueSubsystem.cs | 2 +- .../VersionSubsystem.cs | 6 ++-- 14 files changed, 65 insertions(+), 80 deletions(-) delete mode 100644 src/System.CommandLine.Subsystems/CliExit.cs diff --git a/src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs b/src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs index d4db61f884..70ab7b2a24 100644 --- a/src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs +++ b/src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs @@ -10,11 +10,11 @@ internal class AlternateSubsystems { internal class AlternateVersion : VersionSubsystem { - protected override CliExit Execute(PipelineResult pipelineResult) + protected override PipelineResult Execute(PipelineResult pipelineResult) { pipelineResult.ConsoleHack.WriteLine($"***{CliExecutable.ExecutableVersion}***"); - pipelineResult.AlreadyHandled = true; - return CliExit.SuccessfullyHandled(pipelineResult.ParseResult); + pipelineResult.SetSuccess(); + return pipelineResult; } } @@ -29,12 +29,13 @@ public VersionThatUsesHelpData(CliSymbol symbol) private CliSymbol Symbol { get; } - protected override CliExit Execute(PipelineResult pipelineResult) + protected override PipelineResult Execute(PipelineResult pipelineResult) { TryGetAnnotation(Symbol, HelpAnnotations.Description, out string? description); pipelineResult.ConsoleHack.WriteLine(description); pipelineResult.AlreadyHandled = true; - return CliExit.SuccessfullyHandled(pipelineResult.ParseResult); + pipelineResult.SetSuccess(); + return pipelineResult; } } @@ -51,16 +52,16 @@ protected override CliConfiguration Initialize(InitializationContext context) return base.Initialize(context); } - protected override CliExit Execute(PipelineResult pipelineResult) + protected override PipelineResult Execute(PipelineResult pipelineResult) { ExecutionWasRun = true; return base.Execute(pipelineResult); } - protected override CliExit TearDown(CliExit cliExit) + protected override PipelineResult TearDown(PipelineResult pipelineResult) { TeardownWasRun = true; - return base.TearDown(cliExit); + return base.TearDown(pipelineResult); } } diff --git a/src/System.CommandLine.Subsystems.Tests/PipelineTests.cs b/src/System.CommandLine.Subsystems.Tests/PipelineTests.cs index 27308a20e4..62f1ac8348 100644 --- a/src/System.CommandLine.Subsystems.Tests/PipelineTests.cs +++ b/src/System.CommandLine.Subsystems.Tests/PipelineTests.cs @@ -43,7 +43,7 @@ public void Subsystem_runs_in_pipeline_only_when_requested(string input, bool sh var exit = pipeline.Execute(GetNewTestConfiguration(), input, console); exit.ExitCode.Should().Be(0); - exit.Handled.Should().Be(shouldRun); + exit.AlreadyHandled.Should().Be(shouldRun); if (shouldRun) { console.GetBuffer().Trim().Should().Be(TestData.AssemblyVersionString); @@ -61,7 +61,7 @@ public void Subsystem_runs_with_explicit_parse_only_when_requested(string input, var exit = pipeline.Execute(result, input, console); exit.ExitCode.Should().Be(0); - exit.Handled.Should().Be(shouldRun); + exit.AlreadyHandled.Should().Be(shouldRun); if (shouldRun) { console.GetBuffer().Trim().Should().Be(TestData.AssemblyVersionString); @@ -79,7 +79,7 @@ public void Subsystem_runs_initialize_and_teardown_when_requested(string input, var exit = pipeline.Execute(GetNewTestConfiguration(), input, console); exit.ExitCode.Should().Be(0); - exit.Handled.Should().Be(shouldRun); + exit.AlreadyHandled.Should().Be(shouldRun); versionSubsystem.InitializationWasRun.Should().BeTrue(); versionSubsystem.ExecutionWasRun.Should().Be(shouldRun); versionSubsystem.TeardownWasRun.Should().BeTrue(); @@ -109,7 +109,7 @@ public void Subsystem_works_without_pipeline(string input, bool shouldRun) var exit = Subsystem.Execute(versionSubsystem, parseResult, input, console); exit.Should().NotBeNull(); exit.ExitCode.Should().Be(0); - exit.Handled.Should().BeTrue(); + exit.AlreadyHandled.Should().BeTrue(); console.GetBuffer().Trim().Should().Be(TestData.AssemblyVersionString); } } @@ -132,7 +132,7 @@ public void Subsystem_works_without_pipeline_style2(string input, bool shouldRun var exit = Subsystem.ExecuteIfNeeded(versionSubsystem, parseResult, input, console); exit.ExitCode.Should().Be(0); - exit.Handled.Should().Be(shouldRun); + exit.AlreadyHandled.Should().Be(shouldRun); console.GetBuffer().Trim().Should().Be(expectedVersion); } diff --git a/src/System.CommandLine.Subsystems.Tests/VersionFunctionalTests.cs b/src/System.CommandLine.Subsystems.Tests/VersionFunctionalTests.cs index dfb5681a53..8804cca62e 100644 --- a/src/System.CommandLine.Subsystems.Tests/VersionFunctionalTests.cs +++ b/src/System.CommandLine.Subsystems.Tests/VersionFunctionalTests.cs @@ -26,7 +26,7 @@ public void When_the_version_option_is_specified_then_the_version_is_written_to_ var exit = pipeline.Execute(configuration, "-v", consoleHack); exit.ExitCode.Should().Be(0); - exit.Handled.Should().BeTrue(); + exit.AlreadyHandled.Should().BeTrue(); consoleHack.GetBuffer().Should().Be($"{version}{newLine}"); } diff --git a/src/System.CommandLine.Subsystems/CliExit.cs b/src/System.CommandLine.Subsystems/CliExit.cs deleted file mode 100644 index be1190675f..0000000000 --- a/src/System.CommandLine.Subsystems/CliExit.cs +++ /dev/null @@ -1,35 +0,0 @@ -// 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.Subsystems; - -namespace System.CommandLine; - - // TODO: Consider what info is needed after invocation. If it's the whole pipeline context, consider collapsing this with that class. -public class CliExit -{ - internal CliExit(PipelineResult pipelineResult) - : this(pipelineResult.ParseResult, pipelineResult.AlreadyHandled, pipelineResult.ExitCode) - { } - - private CliExit(ParseResult? parseResult, bool handled, int exitCode) - { - ExitCode = exitCode; - Handled = handled; - ParseResult = parseResult; - } - public ParseResult? ParseResult { get; set; } - - public int ExitCode { get; } - - public static implicit operator int(CliExit cliExit) => cliExit.ExitCode; - - public static implicit operator bool(CliExit cliExit) => !cliExit.Handled; - - - public bool Handled { get; } - - public static CliExit NotRun(ParseResult? parseResult) => new(parseResult, false, 0); - - public static CliExit SuccessfullyHandled(ParseResult? parseResult) => new(parseResult, true, 0); -} diff --git a/src/System.CommandLine.Subsystems/CompletionSubsystem.cs b/src/System.CommandLine.Subsystems/CompletionSubsystem.cs index 3ec5b166da..47ed128053 100644 --- a/src/System.CommandLine.Subsystems/CompletionSubsystem.cs +++ b/src/System.CommandLine.Subsystems/CompletionSubsystem.cs @@ -18,9 +18,10 @@ protected internal override bool GetIsActivated(ParseResult? parseResult) ? false : false; - protected internal override CliExit Execute(PipelineResult pipelineResult) + protected internal override PipelineResult Execute(PipelineResult pipelineResult) { pipelineResult.ConsoleHack.WriteLine("Not yet implemented"); - return CliExit.SuccessfullyHandled(pipelineResult.ParseResult); + pipelineResult.SetSuccess(); + return pipelineResult; } } diff --git a/src/System.CommandLine.Subsystems/Directives/DiagramSubsystem.cs b/src/System.CommandLine.Subsystems/Directives/DiagramSubsystem.cs index 3c0bdd1d49..e9a19ce2b0 100644 --- a/src/System.CommandLine.Subsystems/Directives/DiagramSubsystem.cs +++ b/src/System.CommandLine.Subsystems/Directives/DiagramSubsystem.cs @@ -13,14 +13,15 @@ public class DiagramSubsystem( IAnnotationProvider? annotationProvider = null) //protected internal override bool GetIsActivated(ParseResult? parseResult) // => parseResult is not null && option is not null && parseResult.GetValue(option); - protected internal override CliExit Execute(PipelineResult pipelineResult) + protected internal override PipelineResult Execute(PipelineResult pipelineResult) { // Gather locations //var locations = pipelineResult.ParseResult.LocationMap // .Concat(Map(pipelineResult.ParseResult.Configuration.PreProcessedLocations)); pipelineResult.ConsoleHack.WriteLine("Output diagram"); - return CliExit.SuccessfullyHandled(pipelineResult.ParseResult); + pipelineResult.SetSuccess(); + return pipelineResult; } diff --git a/src/System.CommandLine.Subsystems/ErrorReportingSubsystem.cs b/src/System.CommandLine.Subsystems/ErrorReportingSubsystem.cs index 0948ac0176..a7c0f81d24 100644 --- a/src/System.CommandLine.Subsystems/ErrorReportingSubsystem.cs +++ b/src/System.CommandLine.Subsystems/ErrorReportingSubsystem.cs @@ -23,14 +23,15 @@ 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(PipelineResult pipelineResult) + protected internal override PipelineResult Execute(PipelineResult pipelineResult) { var _ = pipelineResult.ParseResult ?? throw new ArgumentException("The parse result has not been set", nameof(pipelineResult)); Report(pipelineResult.ConsoleHack, pipelineResult.ParseResult.Errors); - return CliExit.SuccessfullyHandled(pipelineResult.ParseResult); + pipelineResult.SetSuccess(); + return pipelineResult; } public void Report(ConsoleHack consoleHack, IReadOnlyList errors) diff --git a/src/System.CommandLine.Subsystems/HelpSubsystem.cs b/src/System.CommandLine.Subsystems/HelpSubsystem.cs index 3be155a18b..2156f59b7b 100644 --- a/src/System.CommandLine.Subsystems/HelpSubsystem.cs +++ b/src/System.CommandLine.Subsystems/HelpSubsystem.cs @@ -34,11 +34,12 @@ protected internal override CliConfiguration Initialize(InitializationContext co protected internal override bool GetIsActivated(ParseResult? parseResult) => parseResult is not null && parseResult.GetValue(HelpOption); - protected internal override CliExit Execute(PipelineResult pipelineResult) + protected internal override PipelineResult Execute(PipelineResult pipelineResult) { // TODO: Match testable output pattern pipelineResult.ConsoleHack.WriteLine("Help me!"); - return CliExit.SuccessfullyHandled(pipelineResult.ParseResult); + pipelineResult.SetSuccess(); + return pipelineResult; } public bool TryGetDescription (CliSymbol symbol, out string? description) diff --git a/src/System.CommandLine.Subsystems/Pipeline.cs b/src/System.CommandLine.Subsystems/Pipeline.cs index 18298cea7b..cc6bedccea 100644 --- a/src/System.CommandLine.Subsystems/Pipeline.cs +++ b/src/System.CommandLine.Subsystems/Pipeline.cs @@ -51,20 +51,20 @@ public ParseResult Parse(CliConfiguration configuration, IReadOnlyList a return parseResult; } - public CliExit Execute(CliConfiguration configuration, string rawInput, ConsoleHack? consoleHack = null) + public PipelineResult Execute(CliConfiguration configuration, string rawInput, ConsoleHack? consoleHack = null) => Execute(configuration, CliParser.SplitCommandLine(rawInput).ToArray(), rawInput, consoleHack); - public CliExit Execute(CliConfiguration configuration, string[] args, string rawInput, ConsoleHack? consoleHack = null) + public PipelineResult Execute(CliConfiguration configuration, string[] args, string rawInput, ConsoleHack? consoleHack = null) { - var cliExit = Execute(Parse(configuration, args), rawInput, consoleHack); - return TearDownSubsystems(cliExit); + var pipelineResult = Execute(Parse(configuration, args), rawInput, consoleHack); + return TearDownSubsystems(pipelineResult); } - public CliExit Execute(ParseResult parseResult, string rawInput, ConsoleHack? consoleHack = null) + public PipelineResult Execute(ParseResult parseResult, string rawInput, ConsoleHack? consoleHack = null) { var pipelineResult = new PipelineResult(parseResult, rawInput, this, consoleHack ?? new ConsoleHack()); ExecuteSubsystems(pipelineResult); - return new CliExit(pipelineResult); + return pipelineResult; } // TODO: Consider whether this should be public. It would simplify testing, but would it do anything else @@ -95,18 +95,18 @@ protected virtual void InitializeSubsystems(InitializationContext context) /// Perform any cleanup operations /// /// The context of the current execution - protected virtual CliExit TearDownSubsystems(CliExit cliExit) + protected virtual PipelineResult TearDownSubsystems(PipelineResult pipelineResult) { - // TODO: Work on this design as the last cliExit wins and they may not all be well behaved + // TODO: Work on this design as the last pipelineResult wins and they may not all be well behaved var subsystems = Subsystems.Reverse(); foreach (var subsystem in subsystems) { if (subsystem is not null) { - cliExit = subsystem.TearDown(cliExit); + pipelineResult = subsystem.TearDown(pipelineResult); } } - return cliExit; + return pipelineResult; } protected virtual void ExecuteSubsystems(PipelineResult pipelineResult) diff --git a/src/System.CommandLine.Subsystems/Subsystems/CliSubsystem.cs b/src/System.CommandLine.Subsystems/Subsystems/CliSubsystem.cs index fdc22d5aae..99a821515e 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/CliSubsystem.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/CliSubsystem.cs @@ -66,9 +66,12 @@ protected internal bool TryGetAnnotation(CliSymbol symbol, AnnotationId< /// Executes the behavior of the subsystem. For example, help would write information to the console. /// /// The context contains data like the ParseResult, and allows setting of values like whether execution was handled and the CLI should terminate - /// A CliExit object with information such as whether the CLI should terminate - protected internal virtual CliExit Execute(PipelineResult pipelineResult) - => CliExit.NotRun(pipelineResult.ParseResult); + /// A PipelineResult object with information such as whether the CLI should terminate + protected internal virtual PipelineResult Execute(PipelineResult pipelineResult) + { + pipelineResult.NotRun(pipelineResult.ParseResult); + return pipelineResult; + } internal PipelineResult ExecuteIfNeeded(PipelineResult pipelineResult) => ExecuteIfNeeded(pipelineResult.ParseResult, pipelineResult); @@ -111,7 +114,7 @@ protected internal virtual CliConfiguration Initialize(InitializationContext con => context.Configuration; // TODO: Determine if this is needed. - protected internal virtual CliExit TearDown(CliExit cliExit) - => cliExit; + protected internal virtual PipelineResult TearDown(PipelineResult pipelineResult) + => pipelineResult; } diff --git a/src/System.CommandLine.Subsystems/Subsystems/PipelineResult.cs b/src/System.CommandLine.Subsystems/Subsystems/PipelineResult.cs index 00c02bc9ac..2c9e56d73a 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/PipelineResult.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/PipelineResult.cs @@ -1,6 +1,8 @@ // 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. + + namespace System.CommandLine.Subsystems; public class PipelineResult(ParseResult? parseResult, string rawInput, Pipeline? pipeline, ConsoleHack? consoleHack = null) @@ -13,4 +15,14 @@ public class PipelineResult(ParseResult? parseResult, string rawInput, Pipeline? public bool AlreadyHandled { get; set; } public int ExitCode { get; set; } + public void NotRun(ParseResult? parseResult) + { + // no op because defaults are false and 0 + } + + public void SetSuccess() + { + AlreadyHandled = true; + ExitCode = 0; + } } diff --git a/src/System.CommandLine.Subsystems/Subsystems/Subsystem.cs b/src/System.CommandLine.Subsystems/Subsystems/Subsystem.cs index 440c313ef3..c641982d7e 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/Subsystem.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/Subsystem.cs @@ -8,16 +8,16 @@ public class Subsystem public static void Initialize(CliSubsystem subsystem, CliConfiguration configuration, IReadOnlyList args) => subsystem.Initialize(new InitializationContext(configuration, args)); - public static CliExit Execute(CliSubsystem subsystem, PipelineResult pipelineResult) + public static PipelineResult Execute(CliSubsystem subsystem, PipelineResult pipelineResult) => subsystem.Execute(pipelineResult); public static bool GetIsActivated(CliSubsystem subsystem, ParseResult parseResult) => subsystem.GetIsActivated(parseResult); - public static CliExit ExecuteIfNeeded(CliSubsystem subsystem, ParseResult parseResult, string rawInput, ConsoleHack? consoleHack = null) - => new(subsystem.ExecuteIfNeeded(new PipelineResult(parseResult, rawInput, null, consoleHack))); + public static PipelineResult ExecuteIfNeeded(CliSubsystem subsystem, ParseResult parseResult, string rawInput, ConsoleHack? consoleHack = null) + => subsystem.ExecuteIfNeeded(new PipelineResult(parseResult, rawInput, null, consoleHack)); - public static CliExit Execute(CliSubsystem subsystem, ParseResult parseResult, string rawInput, ConsoleHack? consoleHack = null) + public static PipelineResult Execute(CliSubsystem subsystem, ParseResult parseResult, string rawInput, ConsoleHack? consoleHack = null) => subsystem.Execute(new PipelineResult(parseResult, rawInput, null, consoleHack)); diff --git a/src/System.CommandLine.Subsystems/ValueSubsystem.cs b/src/System.CommandLine.Subsystems/ValueSubsystem.cs index 6152f019af..92339ea673 100644 --- a/src/System.CommandLine.Subsystems/ValueSubsystem.cs +++ b/src/System.CommandLine.Subsystems/ValueSubsystem.cs @@ -29,7 +29,7 @@ protected internal override bool GetIsActivated(ParseResult? parseResult) /// /// Note to inheritors: Call base for all ValueSubsystem methods that you override to ensure correct behavior /// - protected internal override CliExit Execute(PipelineResult pipelineResult) + protected internal override PipelineResult Execute(PipelineResult pipelineResult) { parseResult ??= pipelineResult.ParseResult; return base.Execute(pipelineResult); diff --git a/src/System.CommandLine.Subsystems/VersionSubsystem.cs b/src/System.CommandLine.Subsystems/VersionSubsystem.cs index c538a896c4..7a06003203 100644 --- a/src/System.CommandLine.Subsystems/VersionSubsystem.cs +++ b/src/System.CommandLine.Subsystems/VersionSubsystem.cs @@ -49,15 +49,15 @@ protected internal override CliConfiguration Initialize(InitializationContext co protected internal override bool GetIsActivated(ParseResult? parseResult) => parseResult is not null && parseResult.GetValue("--version"); - protected internal override CliExit Execute(PipelineResult pipelineResult) + protected internal override PipelineResult Execute(PipelineResult pipelineResult) { var subsystemVersion = SpecificVersion; var version = subsystemVersion is null ? CliExecutable.ExecutableVersion : subsystemVersion; pipelineResult.ConsoleHack.WriteLine(version); - pipelineResult.AlreadyHandled = true; - return CliExit.SuccessfullyHandled(pipelineResult.ParseResult); + pipelineResult.SetSuccess(); + return pipelineResult; } } From 7013b470f684ef3c264d454bc7ccd8cc09fafbc4 Mon Sep 17 00:00:00 2001 From: KathleenDollard Date: Thu, 23 May 2024 13:27:39 -0400 Subject: [PATCH 2/2] Fluent style obscures that there is a single `PipelineResult` that is mutated. --- .../AlternateSubsystems.cs | 18 ++++++++---------- .../CompletionSubsystem.cs | 3 +-- .../Directives/DiagramSubsystem.cs | 3 +-- .../Directives/DirectiveSubsystem.cs | 4 +--- .../Directives/ResponseSubsystem.cs | 7 ++----- .../ErrorReportingSubsystem.cs | 3 +-- .../HelpSubsystem.cs | 11 +++-------- src/System.CommandLine.Subsystems/Pipeline.cs | 10 +++++----- .../Subsystems/CliSubsystem.cs | 15 ++++++--------- .../Subsystems/Subsystem.cs | 17 ++++++++++++----- .../ValueSubsystem.cs | 4 ++-- .../VersionSubsystem.cs | 7 ++----- 12 files changed, 44 insertions(+), 58 deletions(-) diff --git a/src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs b/src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs index 70ab7b2a24..abab37351b 100644 --- a/src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs +++ b/src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs @@ -10,11 +10,10 @@ internal class AlternateSubsystems { internal class AlternateVersion : VersionSubsystem { - protected override PipelineResult Execute(PipelineResult pipelineResult) + protected override void Execute(PipelineResult pipelineResult) { pipelineResult.ConsoleHack.WriteLine($"***{CliExecutable.ExecutableVersion}***"); pipelineResult.SetSuccess(); - return pipelineResult; } } @@ -29,13 +28,12 @@ public VersionThatUsesHelpData(CliSymbol symbol) private CliSymbol Symbol { get; } - protected override PipelineResult Execute(PipelineResult pipelineResult) + protected override void Execute(PipelineResult pipelineResult) { TryGetAnnotation(Symbol, HelpAnnotations.Description, out string? description); pipelineResult.ConsoleHack.WriteLine(description); pipelineResult.AlreadyHandled = true; pipelineResult.SetSuccess(); - return pipelineResult; } } @@ -45,23 +43,23 @@ internal class VersionWithInitializeAndTeardown : VersionSubsystem internal bool ExecutionWasRun; internal bool TeardownWasRun; - protected override CliConfiguration Initialize(InitializationContext context) + protected override void Initialize(InitializationContext context) { + base.Initialize(context); // marker hack needed because ConsoleHack not available in initialization InitializationWasRun = true; - return base.Initialize(context); } - protected override PipelineResult Execute(PipelineResult pipelineResult) + protected override void Execute(PipelineResult pipelineResult) { ExecutionWasRun = true; - return base.Execute(pipelineResult); + base.Execute(pipelineResult); } - protected override PipelineResult TearDown(PipelineResult pipelineResult) + protected override void TearDown(PipelineResult pipelineResult) { TeardownWasRun = true; - return base.TearDown(pipelineResult); + base.TearDown(pipelineResult); } } diff --git a/src/System.CommandLine.Subsystems/CompletionSubsystem.cs b/src/System.CommandLine.Subsystems/CompletionSubsystem.cs index 47ed128053..434d4e1934 100644 --- a/src/System.CommandLine.Subsystems/CompletionSubsystem.cs +++ b/src/System.CommandLine.Subsystems/CompletionSubsystem.cs @@ -18,10 +18,9 @@ protected internal override bool GetIsActivated(ParseResult? parseResult) ? false : false; - protected internal override PipelineResult Execute(PipelineResult pipelineResult) + protected internal override void Execute(PipelineResult pipelineResult) { pipelineResult.ConsoleHack.WriteLine("Not yet implemented"); pipelineResult.SetSuccess(); - return pipelineResult; } } diff --git a/src/System.CommandLine.Subsystems/Directives/DiagramSubsystem.cs b/src/System.CommandLine.Subsystems/Directives/DiagramSubsystem.cs index e9a19ce2b0..c31ea6e2a9 100644 --- a/src/System.CommandLine.Subsystems/Directives/DiagramSubsystem.cs +++ b/src/System.CommandLine.Subsystems/Directives/DiagramSubsystem.cs @@ -13,7 +13,7 @@ public class DiagramSubsystem( IAnnotationProvider? annotationProvider = null) //protected internal override bool GetIsActivated(ParseResult? parseResult) // => parseResult is not null && option is not null && parseResult.GetValue(option); - protected internal override PipelineResult Execute(PipelineResult pipelineResult) + protected internal override void Execute(PipelineResult pipelineResult) { // Gather locations //var locations = pipelineResult.ParseResult.LocationMap @@ -21,7 +21,6 @@ protected internal override PipelineResult Execute(PipelineResult pipelineResult pipelineResult.ConsoleHack.WriteLine("Output diagram"); pipelineResult.SetSuccess(); - return pipelineResult; } diff --git a/src/System.CommandLine.Subsystems/Directives/DirectiveSubsystem.cs b/src/System.CommandLine.Subsystems/Directives/DirectiveSubsystem.cs index 6b4b7d9670..efb51bceee 100644 --- a/src/System.CommandLine.Subsystems/Directives/DirectiveSubsystem.cs +++ b/src/System.CommandLine.Subsystems/Directives/DirectiveSubsystem.cs @@ -19,7 +19,7 @@ public DirectiveSubsystem(string name, SubsystemKind kind, IAnnotationProvider? Id = id ?? name; } - protected internal override CliConfiguration Initialize(InitializationContext context) + protected internal override void Initialize(InitializationContext context) { for (int i = 0; i < context.Args.Count; i++) { @@ -50,8 +50,6 @@ protected internal override CliConfiguration Initialize(InitializationContext co break; } } - - return context.Configuration; } protected internal override bool GetIsActivated(ParseResult? parseResult) diff --git a/src/System.CommandLine.Subsystems/Directives/ResponseSubsystem.cs b/src/System.CommandLine.Subsystems/Directives/ResponseSubsystem.cs index ed43c8d626..66a1fd6b79 100644 --- a/src/System.CommandLine.Subsystems/Directives/ResponseSubsystem.cs +++ b/src/System.CommandLine.Subsystems/Directives/ResponseSubsystem.cs @@ -9,11 +9,8 @@ namespace System.CommandLine.Directives; public class ResponseSubsystem() : CliSubsystem("Response", SubsystemKind.Response, null) { - protected internal override CliConfiguration Initialize(InitializationContext context) - { - context.Configuration.ResponseFileTokenReplacer = Replacer; - return context.Configuration; - } + protected internal override void Initialize(InitializationContext context) + => context.Configuration.ResponseFileTokenReplacer = Replacer; public static (List? tokens, List? errors) Replacer(string responseSourceName) { diff --git a/src/System.CommandLine.Subsystems/ErrorReportingSubsystem.cs b/src/System.CommandLine.Subsystems/ErrorReportingSubsystem.cs index a7c0f81d24..9effb620f0 100644 --- a/src/System.CommandLine.Subsystems/ErrorReportingSubsystem.cs +++ b/src/System.CommandLine.Subsystems/ErrorReportingSubsystem.cs @@ -23,7 +23,7 @@ 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 PipelineResult Execute(PipelineResult pipelineResult) + protected internal override void Execute(PipelineResult pipelineResult) { var _ = pipelineResult.ParseResult ?? throw new ArgumentException("The parse result has not been set", nameof(pipelineResult)); @@ -31,7 +31,6 @@ protected internal override PipelineResult Execute(PipelineResult pipelineResult Report(pipelineResult.ConsoleHack, pipelineResult.ParseResult.Errors); pipelineResult.SetSuccess(); - return pipelineResult; } public void Report(ConsoleHack consoleHack, IReadOnlyList errors) diff --git a/src/System.CommandLine.Subsystems/HelpSubsystem.cs b/src/System.CommandLine.Subsystems/HelpSubsystem.cs index 2156f59b7b..b4a922420b 100644 --- a/src/System.CommandLine.Subsystems/HelpSubsystem.cs +++ b/src/System.CommandLine.Subsystems/HelpSubsystem.cs @@ -24,22 +24,17 @@ public class HelpSubsystem(IAnnotationProvider? annotationProvider = null) Arity = ArgumentArity.Zero }; - protected internal override CliConfiguration Initialize(InitializationContext context) - { - context.Configuration.RootCommand.Add(HelpOption); - - return context.Configuration; - } + protected internal override void Initialize(InitializationContext context) + => context.Configuration.RootCommand.Add(HelpOption); protected internal override bool GetIsActivated(ParseResult? parseResult) => parseResult is not null && parseResult.GetValue(HelpOption); - protected internal override PipelineResult Execute(PipelineResult pipelineResult) + protected internal override void Execute(PipelineResult pipelineResult) { // TODO: Match testable output pattern pipelineResult.ConsoleHack.WriteLine("Help me!"); pipelineResult.SetSuccess(); - return pipelineResult; } public bool TryGetDescription (CliSymbol symbol, out string? description) diff --git a/src/System.CommandLine.Subsystems/Pipeline.cs b/src/System.CommandLine.Subsystems/Pipeline.cs index cc6bedccea..4bceb1db18 100644 --- a/src/System.CommandLine.Subsystems/Pipeline.cs +++ b/src/System.CommandLine.Subsystems/Pipeline.cs @@ -57,7 +57,8 @@ public PipelineResult Execute(CliConfiguration configuration, string rawInput, C public PipelineResult Execute(CliConfiguration configuration, string[] args, string rawInput, ConsoleHack? consoleHack = null) { var pipelineResult = Execute(Parse(configuration, args), rawInput, consoleHack); - return TearDownSubsystems(pipelineResult); + TearDownSubsystems(pipelineResult); + return pipelineResult; } public PipelineResult Execute(ParseResult parseResult, string rawInput, ConsoleHack? consoleHack = null) @@ -95,7 +96,7 @@ protected virtual void InitializeSubsystems(InitializationContext context) /// Perform any cleanup operations /// /// The context of the current execution - protected virtual PipelineResult TearDownSubsystems(PipelineResult pipelineResult) + protected virtual void TearDownSubsystems(PipelineResult pipelineResult) { // TODO: Work on this design as the last pipelineResult wins and they may not all be well behaved var subsystems = Subsystems.Reverse(); @@ -103,10 +104,9 @@ protected virtual PipelineResult TearDownSubsystems(PipelineResult pipelineResul { if (subsystem is not null) { - pipelineResult = subsystem.TearDown(pipelineResult); + subsystem.TearDown(pipelineResult); } } - return pipelineResult; } protected virtual void ExecuteSubsystems(PipelineResult pipelineResult) @@ -117,7 +117,7 @@ protected virtual void ExecuteSubsystems(PipelineResult pipelineResult) { if (subsystem is not null) { - pipelineResult = subsystem.ExecuteIfNeeded(pipelineResult); + subsystem.ExecuteIfNeeded(pipelineResult); } } } diff --git a/src/System.CommandLine.Subsystems/Subsystems/CliSubsystem.cs b/src/System.CommandLine.Subsystems/Subsystems/CliSubsystem.cs index 99a821515e..157a45f441 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/CliSubsystem.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/CliSubsystem.cs @@ -67,11 +67,8 @@ protected internal bool TryGetAnnotation(CliSymbol symbol, AnnotationId< /// /// The context contains data like the ParseResult, and allows setting of values like whether execution was handled and the CLI should terminate /// A PipelineResult object with information such as whether the CLI should terminate - protected internal virtual PipelineResult Execute(PipelineResult pipelineResult) - { - pipelineResult.NotRun(pipelineResult.ParseResult); - return pipelineResult; - } + protected internal virtual void Execute(PipelineResult pipelineResult) + => pipelineResult.NotRun(pipelineResult.ParseResult); internal PipelineResult ExecuteIfNeeded(PipelineResult pipelineResult) => ExecuteIfNeeded(pipelineResult.ParseResult, pipelineResult); @@ -110,11 +107,11 @@ internal PipelineResult ExecuteIfNeeded(ParseResult? parseResult, PipelineResult /// True if parsing should continue // there might be a better design that supports a message // TODO: Because of this and similar usage, consider combining CLI declaration and config. ArgParse calls this the parser, which I like // TODO: Why does Intitialize return a configuration? - protected internal virtual CliConfiguration Initialize(InitializationContext context) - => context.Configuration; + protected internal virtual void Initialize(InitializationContext context) + {} // TODO: Determine if this is needed. - protected internal virtual PipelineResult TearDown(PipelineResult pipelineResult) - => pipelineResult; + protected internal virtual void TearDown(PipelineResult pipelineResult) + {} } diff --git a/src/System.CommandLine.Subsystems/Subsystems/Subsystem.cs b/src/System.CommandLine.Subsystems/Subsystems/Subsystem.cs index c641982d7e..e55d28a7d7 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/Subsystem.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/Subsystem.cs @@ -8,7 +8,7 @@ public class Subsystem public static void Initialize(CliSubsystem subsystem, CliConfiguration configuration, IReadOnlyList args) => subsystem.Initialize(new InitializationContext(configuration, args)); - public static PipelineResult Execute(CliSubsystem subsystem, PipelineResult pipelineResult) + public static void Execute(CliSubsystem subsystem, PipelineResult pipelineResult) => subsystem.Execute(pipelineResult); public static bool GetIsActivated(CliSubsystem subsystem, ParseResult parseResult) @@ -18,12 +18,19 @@ public static PipelineResult ExecuteIfNeeded(CliSubsystem subsystem, ParseResult => subsystem.ExecuteIfNeeded(new PipelineResult(parseResult, rawInput, null, consoleHack)); public static PipelineResult Execute(CliSubsystem subsystem, ParseResult parseResult, string rawInput, ConsoleHack? consoleHack = null) - => subsystem.Execute(new PipelineResult(parseResult, rawInput, null, consoleHack)); - + { + var pipelineResult = new PipelineResult(parseResult, rawInput,null, consoleHack); + subsystem.Execute(pipelineResult); + return pipelineResult; + } internal static PipelineResult ExecuteIfNeeded(CliSubsystem subsystem, ParseResult parseResult, string rawInput, ConsoleHack? consoleHack, PipelineResult? pipelineResult = null) - => subsystem.ExecuteIfNeeded(pipelineResult ?? new PipelineResult(parseResult, rawInput, null, consoleHack)); + { + pipelineResult ??= new PipelineResult(parseResult, rawInput, null, consoleHack); + subsystem.ExecuteIfNeeded(pipelineResult ); + return pipelineResult; + } - internal static PipelineResult ExecuteIfNeeded(CliSubsystem subsystem, PipelineResult pipelineResult) + internal static void ExecuteIfNeeded(CliSubsystem subsystem, PipelineResult pipelineResult) => subsystem.ExecuteIfNeeded(pipelineResult); } diff --git a/src/System.CommandLine.Subsystems/ValueSubsystem.cs b/src/System.CommandLine.Subsystems/ValueSubsystem.cs index 92339ea673..2257743ccd 100644 --- a/src/System.CommandLine.Subsystems/ValueSubsystem.cs +++ b/src/System.CommandLine.Subsystems/ValueSubsystem.cs @@ -29,10 +29,10 @@ protected internal override bool GetIsActivated(ParseResult? parseResult) /// /// Note to inheritors: Call base for all ValueSubsystem methods that you override to ensure correct behavior /// - protected internal override PipelineResult Execute(PipelineResult pipelineResult) + protected internal override void Execute(PipelineResult pipelineResult) { parseResult ??= pipelineResult.ParseResult; - return base.Execute(pipelineResult); + base.Execute(pipelineResult); } private void SetValue(CliSymbol symbol, object? value) diff --git a/src/System.CommandLine.Subsystems/VersionSubsystem.cs b/src/System.CommandLine.Subsystems/VersionSubsystem.cs index 7a06003203..8cb07d116d 100644 --- a/src/System.CommandLine.Subsystems/VersionSubsystem.cs +++ b/src/System.CommandLine.Subsystems/VersionSubsystem.cs @@ -34,22 +34,20 @@ public string? SpecificVersion ?.GetCustomAttribute() ?.InformationalVersion; - protected internal override CliConfiguration Initialize(InitializationContext context) + protected internal override void Initialize(InitializationContext context) { var option = new CliOption("--version", ["-v"]) { Arity = ArgumentArity.Zero }; context.Configuration.RootCommand.Add(option); - - return context.Configuration; } // TODO: Stash option rather than using string protected internal override bool GetIsActivated(ParseResult? parseResult) => parseResult is not null && parseResult.GetValue("--version"); - protected internal override PipelineResult Execute(PipelineResult pipelineResult) + protected internal override void Execute(PipelineResult pipelineResult) { var subsystemVersion = SpecificVersion; var version = subsystemVersion is null @@ -57,7 +55,6 @@ protected internal override PipelineResult Execute(PipelineResult pipelineResult : subsystemVersion; pipelineResult.ConsoleHack.WriteLine(version); pipelineResult.SetSuccess(); - return pipelineResult; } }