From f9986d7373ee5759e27afc04ed07f02344f2faa5 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 17 Mar 2023 17:13:32 +0100 Subject: [PATCH 1/4] merge two ConsoleExtensions types into one, remove unused IConsole argument --- .../Builder/CommandLineBuilderExtensions.cs | 6 +- src/System.CommandLine/ConsoleExtensions.cs | 50 ++++++++++++++++ .../IO/ConsoleExtensions.cs | 58 ------------------- .../Invocation/ParseErrorResult.cs | 6 +- 4 files changed, 56 insertions(+), 64 deletions(-) delete mode 100644 src/System.CommandLine/IO/ConsoleExtensions.cs diff --git a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs index ea3efd76a0..310adb136e 100644 --- a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs +++ b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs @@ -115,13 +115,13 @@ int Default(Exception exception, InvocationContext context) { if (exception is not OperationCanceledException) { - context.Console.ResetTerminalForegroundColor(); - context.Console.SetTerminalForegroundRed(); + ConsoleExtensions.ResetTerminalForegroundColor(); + ConsoleExtensions.SetTerminalForegroundRed(); context.Console.Error.Write(LocalizationResources.ExceptionHandlerHeader()); context.Console.Error.WriteLine(exception.ToString()); - context.Console.ResetTerminalForegroundColor(); + ConsoleExtensions.ResetTerminalForegroundColor(); } return errorExitCode; } diff --git a/src/System.CommandLine/ConsoleExtensions.cs b/src/System.CommandLine/ConsoleExtensions.cs index 2705a8e287..00762a5424 100644 --- a/src/System.CommandLine/ConsoleExtensions.cs +++ b/src/System.CommandLine/ConsoleExtensions.cs @@ -10,6 +10,56 @@ namespace System.CommandLine /// public static class ConsoleExtensions { + private static bool? _isConsoleRedirectionCheckSupported; + + private static bool IsConsoleRedirectionCheckSupported + { + get + { + if (_isConsoleRedirectionCheckSupported is null) + { + try + { + var check = Console.IsOutputRedirected; + _isConsoleRedirectionCheckSupported = true; + } + + catch (PlatformNotSupportedException) + { + _isConsoleRedirectionCheckSupported = false; + } + } + + return _isConsoleRedirectionCheckSupported.Value; + } + } + + internal static void SetTerminalForegroundRed() + { + if (IsConsoleRedirectionCheckSupported && + !Console.IsOutputRedirected) + { + Console.ForegroundColor = ConsoleColor.Red; + } + else if (IsConsoleRedirectionCheckSupported) + { + Console.ForegroundColor = ConsoleColor.Red; + } + } + + internal static void ResetTerminalForegroundColor() + { + if (IsConsoleRedirectionCheckSupported && + !Console.IsOutputRedirected) + { + Console.ResetColor(); + } + else if (IsConsoleRedirectionCheckSupported) + { + Console.ResetColor(); + } + } + /// /// Writes the current string value to the standard output stream. /// diff --git a/src/System.CommandLine/IO/ConsoleExtensions.cs b/src/System.CommandLine/IO/ConsoleExtensions.cs deleted file mode 100644 index 7ec5ab26b4..0000000000 --- a/src/System.CommandLine/IO/ConsoleExtensions.cs +++ /dev/null @@ -1,58 +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. - -namespace System.CommandLine.IO -{ - internal static class ConsoleExtensions - { - private static bool? _isConsoleRedirectionCheckSupported; - - private static bool IsConsoleRedirectionCheckSupported - { - get - { - if (_isConsoleRedirectionCheckSupported is null) - { - try - { - var check = Console.IsOutputRedirected; - _isConsoleRedirectionCheckSupported = true; - } - - catch (PlatformNotSupportedException) - { - _isConsoleRedirectionCheckSupported = false; - } - } - - return _isConsoleRedirectionCheckSupported.Value; - } - } - - internal static void SetTerminalForegroundRed(this IConsole console) - { - if (IsConsoleRedirectionCheckSupported && - !Console.IsOutputRedirected) - { - Console.ForegroundColor = ConsoleColor.Red; - } - else if (IsConsoleRedirectionCheckSupported) - { - Console.ForegroundColor = ConsoleColor.Red; - } - } - - internal static void ResetTerminalForegroundColor(this IConsole console) - { - if (IsConsoleRedirectionCheckSupported && - !Console.IsOutputRedirected) - { - Console.ResetColor(); - } - else if (IsConsoleRedirectionCheckSupported) - { - Console.ResetColor(); - } - } - } -} \ No newline at end of file diff --git a/src/System.CommandLine/Invocation/ParseErrorResult.cs b/src/System.CommandLine/Invocation/ParseErrorResult.cs index 29367c1754..946fca5205 100644 --- a/src/System.CommandLine/Invocation/ParseErrorResult.cs +++ b/src/System.CommandLine/Invocation/ParseErrorResult.cs @@ -12,8 +12,8 @@ internal sealed class ParseErrorResultAction : CliAction { public override int Invoke(InvocationContext context) { - context.Console.ResetTerminalForegroundColor(); - context.Console.SetTerminalForegroundRed(); + ConsoleExtensions.ResetTerminalForegroundColor(); + ConsoleExtensions.SetTerminalForegroundRed(); foreach (var error in context.ParseResult.Errors) { @@ -22,7 +22,7 @@ public override int Invoke(InvocationContext context) context.Console.Error.WriteLine(); - context.Console.ResetTerminalForegroundColor(); + ConsoleExtensions.ResetTerminalForegroundColor(); new HelpOption().Action!.Invoke(context); From 0b9b7f8a64d6b07b0156cf277a340e6b0dd33e30 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 17 Mar 2023 17:17:46 +0100 Subject: [PATCH 2/4] simplify the logic --- src/System.CommandLine/ConsoleExtensions.cs | 27 ++++++--------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/src/System.CommandLine/ConsoleExtensions.cs b/src/System.CommandLine/ConsoleExtensions.cs index 00762a5424..abab679e8f 100644 --- a/src/System.CommandLine/ConsoleExtensions.cs +++ b/src/System.CommandLine/ConsoleExtensions.cs @@ -10,38 +10,32 @@ namespace System.CommandLine /// public static class ConsoleExtensions { - private static bool? _isConsoleRedirectionCheckSupported; + private static bool? _colorsAreSupported; - private static bool IsConsoleRedirectionCheckSupported + private static bool ColorsAreSupported { get { - if (_isConsoleRedirectionCheckSupported is null) + if (_colorsAreSupported is null) { try { - var check = Console.IsOutputRedirected; - _isConsoleRedirectionCheckSupported = true; + _colorsAreSupported = !Console.IsOutputRedirected; } catch (PlatformNotSupportedException) { - _isConsoleRedirectionCheckSupported = false; + _colorsAreSupported = false; } } - return _isConsoleRedirectionCheckSupported.Value; + return _colorsAreSupported.Value; } } internal static void SetTerminalForegroundRed() { - if (IsConsoleRedirectionCheckSupported && - !Console.IsOutputRedirected) - { - Console.ForegroundColor = ConsoleColor.Red; - } - else if (IsConsoleRedirectionCheckSupported) + if (ColorsAreSupported) { Console.ForegroundColor = ConsoleColor.Red; } @@ -49,12 +43,7 @@ internal static void SetTerminalForegroundRed() internal static void ResetTerminalForegroundColor() { - if (IsConsoleRedirectionCheckSupported && - !Console.IsOutputRedirected) - { - Console.ResetColor(); - } - else if (IsConsoleRedirectionCheckSupported) + if (ColorsAreSupported) { Console.ResetColor(); } From adba90d801f8844e3c103525ce64fd19779e4829 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 17 Mar 2023 17:20:05 +0100 Subject: [PATCH 3/4] remove almost unused public methods from ConsoleExtensions, rename it to ConsoleHelpers as there are no extensions anymore --- ...ommandLine_api_is_not_changed.approved.txt | 3 --- .../CancelOnProcessTerminationTests.cs | 2 +- .../TestApps/NativeAOT/Program.cs | 4 ++-- .../TestApps/Trimming/Program.cs | 2 +- .../Builder/CommandLineBuilderExtensions.cs | 6 ++--- ...ConsoleExtensions.cs => ConsoleHelpers.cs} | 23 +------------------ .../Invocation/ParseErrorResult.cs | 6 ++--- 7 files changed, 11 insertions(+), 35 deletions(-) rename src/System.CommandLine/{ConsoleExtensions.cs => ConsoleHelpers.cs} (55%) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index 3b6cc1db73..79d49a6d07 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -102,9 +102,6 @@ System.CommandLine public static class CompletionSourceExtensions public static System.Void Add(this System.Collections.Generic.List>> completionSources, System.Func> completionsDelegate) public static System.Void Add(this System.Collections.Generic.List>> completionSources, System.String[] completions) - public static class ConsoleExtensions - public static System.Void Write(this IConsole console, System.String value) - public static System.Void WriteLine(this IConsole console, System.String value) public class Directive : Symbol .ctor(System.String name) public CliAction Action { get; set; } diff --git a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs index 295ab476e7..ce59fdc250 100644 --- a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs +++ b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs @@ -69,7 +69,7 @@ private sealed class CustomCliAction : CliAction public async override Task InvokeAsync(InvocationContext context, CancellationToken cancellationToken = default) { - context.Console.WriteLine(ChildProcessWaiting); + Console.WriteLine(ChildProcessWaiting); bool infiniteDelay = context.GetValue(InfiniteDelayOption); diff --git a/src/System.CommandLine.Tests/TestApps/NativeAOT/Program.cs b/src/System.CommandLine.Tests/TestApps/NativeAOT/Program.cs index 0200c24157..b81cf144e7 100644 --- a/src/System.CommandLine.Tests/TestApps/NativeAOT/Program.cs +++ b/src/System.CommandLine.Tests/TestApps/NativeAOT/Program.cs @@ -22,8 +22,8 @@ private static int Main(string[] args) void Run(InvocationContext context) { - context.Console.WriteLine($"Bool option: {context.ParseResult.GetValue(boolOption)}"); - context.Console.WriteLine($"String option: {context.ParseResult.GetValue(stringOption)}"); + Console.WriteLine($"Bool option: {context.ParseResult.GetValue(boolOption)}"); + Console.WriteLine($"String option: {context.ParseResult.GetValue(stringOption)}"); } } } \ No newline at end of file diff --git a/src/System.CommandLine.Tests/TestApps/Trimming/Program.cs b/src/System.CommandLine.Tests/TestApps/Trimming/Program.cs index 1bb70d8f88..7254396fa1 100644 --- a/src/System.CommandLine.Tests/TestApps/Trimming/Program.cs +++ b/src/System.CommandLine.Tests/TestApps/Trimming/Program.cs @@ -11,7 +11,7 @@ command.SetAction(context => { - context.Console.Write($"The file you chose was: {context.ParseResult.GetValue(fileArgument)}"); + Console.Write($"The file you chose was: {context.ParseResult.GetValue(fileArgument)}"); }); command.Invoke(args); diff --git a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs index 310adb136e..0d0933f82d 100644 --- a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs +++ b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs @@ -115,13 +115,13 @@ int Default(Exception exception, InvocationContext context) { if (exception is not OperationCanceledException) { - ConsoleExtensions.ResetTerminalForegroundColor(); - ConsoleExtensions.SetTerminalForegroundRed(); + ConsoleHelpers.ResetTerminalForegroundColor(); + ConsoleHelpers.SetTerminalForegroundRed(); context.Console.Error.Write(LocalizationResources.ExceptionHandlerHeader()); context.Console.Error.WriteLine(exception.ToString()); - ConsoleExtensions.ResetTerminalForegroundColor(); + ConsoleHelpers.ResetTerminalForegroundColor(); } return errorExitCode; } diff --git a/src/System.CommandLine/ConsoleExtensions.cs b/src/System.CommandLine/ConsoleHelpers.cs similarity index 55% rename from src/System.CommandLine/ConsoleExtensions.cs rename to src/System.CommandLine/ConsoleHelpers.cs index abab679e8f..13fc10d728 100644 --- a/src/System.CommandLine/ConsoleExtensions.cs +++ b/src/System.CommandLine/ConsoleHelpers.cs @@ -1,14 +1,9 @@ // 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.IO; - namespace System.CommandLine { - /// - /// Provides extension methods for . - /// - public static class ConsoleExtensions + internal static class ConsoleHelpers { private static bool? _colorsAreSupported; @@ -48,21 +43,5 @@ internal static void ResetTerminalForegroundColor() Console.ResetColor(); } } - - /// - /// Writes the current string value to the standard output stream. - /// - /// The console to write to. - /// The value to write. - public static void Write(this IConsole console, string value) => - console.Out.Write(value); - - /// - /// Writes the current string value, followed by the current environment's line terminator, to the standard output stream. - /// - /// The console to write to. - /// The value to write. - public static void WriteLine(this IConsole console, string value) => - console.Out.WriteLine(value); } } \ No newline at end of file diff --git a/src/System.CommandLine/Invocation/ParseErrorResult.cs b/src/System.CommandLine/Invocation/ParseErrorResult.cs index 946fca5205..edbd513666 100644 --- a/src/System.CommandLine/Invocation/ParseErrorResult.cs +++ b/src/System.CommandLine/Invocation/ParseErrorResult.cs @@ -12,8 +12,8 @@ internal sealed class ParseErrorResultAction : CliAction { public override int Invoke(InvocationContext context) { - ConsoleExtensions.ResetTerminalForegroundColor(); - ConsoleExtensions.SetTerminalForegroundRed(); + ConsoleHelpers.ResetTerminalForegroundColor(); + ConsoleHelpers.SetTerminalForegroundRed(); foreach (var error in context.ParseResult.Errors) { @@ -22,7 +22,7 @@ public override int Invoke(InvocationContext context) context.Console.Error.WriteLine(); - ConsoleExtensions.ResetTerminalForegroundColor(); + ConsoleHelpers.ResetTerminalForegroundColor(); new HelpOption().Action!.Invoke(context); From 6de87170ac81727cb854001ce7064a3843b97087 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Sat, 18 Mar 2023 09:23:56 +0100 Subject: [PATCH 4/4] address code review feedback: use more reliable way of checking whether colors are supported: check OS first (in non-throwing way), then check IsOutputRedirected which won't throw at this point --- src/System.CommandLine/ConsoleHelpers.cs | 34 +++++++++--------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/src/System.CommandLine/ConsoleHelpers.cs b/src/System.CommandLine/ConsoleHelpers.cs index 13fc10d728..2684413c89 100644 --- a/src/System.CommandLine/ConsoleHelpers.cs +++ b/src/System.CommandLine/ConsoleHelpers.cs @@ -1,32 +1,24 @@ // 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.Runtime.InteropServices; + namespace System.CommandLine { internal static class ConsoleHelpers { - private static bool? _colorsAreSupported; - - private static bool ColorsAreSupported - { - get - { - if (_colorsAreSupported is null) - { - try - { - _colorsAreSupported = !Console.IsOutputRedirected; - } + private static readonly bool ColorsAreSupported = GetColorsAreSupported(); - catch (PlatformNotSupportedException) - { - _colorsAreSupported = false; - } - } - - return _colorsAreSupported.Value; - } - } + private static bool GetColorsAreSupported() +#if NET7_0_OR_GREATER + => !(OperatingSystem.IsBrowser() || OperatingSystem.IsAndroid() || OperatingSystem.IsIOS() || OperatingSystem.IsTvOS()) +#else + => !(RuntimeInformation.IsOSPlatform(OSPlatform.Create("BROWSER")) + || RuntimeInformation.IsOSPlatform(OSPlatform.Create("ANDROID")) + || RuntimeInformation.IsOSPlatform(OSPlatform.Create("IOS")) + || RuntimeInformation.IsOSPlatform(OSPlatform.Create("TVOS"))) +#endif + && !Console.IsOutputRedirected; internal static void SetTerminalForegroundRed() {