Skip to content

use explicit types for commandline parsing #711

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

use explicit types for commandline parsing #711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7639427
use explicit types for commandline parsing
jmarolf Jun 29, 2020
c487aca
remove redundant argument additions
jmarolf Jun 29, 2020
3eec682
remove unnecassary error checking
jmarolf Jun 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 73 additions & 25 deletions src/FormatCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
using System.CommandLine.Parsing;
using System.Linq;

using Microsoft.Extensions.Logging;

namespace Microsoft.CodeAnalysis.Tools
{
internal static class FormatCommand
Expand All @@ -21,33 +23,24 @@ internal static RootCommand CreateCommandLineOptions()
Arity = ArgumentArity.ZeroOrOne,
Description = Resources.A_path_to_a_solution_file_a_project_file_or_a_folder_containing_a_solution_or_project_file_If_a_path_is_not_specified_then_the_current_directory_is_used
}.LegalFilePathsOnly(),
new Option(new[] { "--folder", "-f" }, Resources.Whether_to_treat_the_workspace_argument_as_a_simple_folder_of_files),
new Option(new[] { "--fix-style", "-fs" }, Resources.Run_code_style_analyzer_and_apply_fixes)
{
Argument = new Argument<string?>("severity") { Arity = ArgumentArity.ZeroOrOne }.FromAmong(SeverityLevels)
},
new Option(new[] { "--fix-analyzers", "-fa" }, Resources.Run_code_style_analyzer_and_apply_fixes)
{
Argument = new Argument<string?>("severity") { Arity = ArgumentArity.ZeroOrOne }.FromAmong(SeverityLevels)
},
new Option(new[] { "--include" }, Resources.A_list_of_relative_file_or_folder_paths_to_include_in_formatting_All_files_are_formatted_if_empty)
{
Argument = new Argument<string[]>(() => Array.Empty<string>())
},
new Option(new[] { "--exclude" }, Resources.A_list_of_relative_file_or_folder_paths_to_exclude_from_formatting)
new Option<bool>(new[] { "--folder", "-f" }, Resources.Whether_to_treat_the_workspace_argument_as_a_simple_folder_of_files),
new Option<DiagnosticSeverity?>(new[] { "--fix-style" }, Resources.Run_code_style_analyzer_and_apply_fixes)
{
Argument = new Argument<string[]>(() => Array.Empty<string>())
Argument = new Argument<DiagnosticSeverity?>(parse: ParseDiagnosticSeverity) { Arity = ArgumentArity.ZeroOrOne }.FromAmong(SeverityLevels)
},
new Option(new[] { "--check" }, Resources.Formats_files_without_saving_changes_to_disk_Terminates_with_a_non_zero_exit_code_if_any_files_were_formatted),
new Option(new[] { "--report" }, Resources.Accepts_a_file_path_which_if_provided_will_produce_a_format_report_json_file_in_the_given_directory)
new Option<DiagnosticSeverity?>(new[] { "--fix-analyzers" }, Resources.Run_code_style_analyzer_and_apply_fixes)
{
Argument = new Argument<string?>(() => null).LegalFilePathsOnly()
Argument = new Argument<DiagnosticSeverity?>(parse: ParseDiagnosticSeverity) { Arity = ArgumentArity.ZeroOrOne }.FromAmong(SeverityLevels)
},
new Option(new[] { "--verbosity", "-v" }, Resources.Set_the_verbosity_level_Allowed_values_are_quiet_minimal_normal_detailed_and_diagnostic)
new Option<string[]>(new[] { "--include" }, getDefaultValue: () => Array.Empty<string>(), description: Resources.A_list_of_relative_file_or_folder_paths_to_include_in_formatting_All_files_are_formatted_if_empty),
new Option<string[]>(new[] { "--exclude" }, getDefaultValue: () => Array.Empty<string>(), description: Resources.A_list_of_relative_file_or_folder_paths_to_exclude_from_formatting),
new Option<bool>(new[] { "--check" }, Resources.Formats_files_without_saving_changes_to_disk_Terminates_with_a_non_zero_exit_code_if_any_files_were_formatted),
new Option<string?>(new[] { "--report" }, getDefaultValue: () => null, description: Resources.Accepts_a_file_path_which_if_provided_will_produce_a_format_report_json_file_in_the_given_directory),
new Option<LogLevel?>(new[] { "--verbosity", "-v" }, Resources.Set_the_verbosity_level_Allowed_values_are_quiet_minimal_normal_detailed_and_diagnostic)
{
Argument = new Argument<string?>() { Arity = ArgumentArity.ExactlyOne }.FromAmong(VerbosityLevels)
Argument = new Argument<LogLevel?>(parse: ParseLogLevel) { Arity = ArgumentArity.ExactlyOne }.FromAmong(VerbosityLevels)
},
new Option(new[] { "--include-generated" }, Resources.Include_generated_code_files_in_formatting_operations)
new Option<bool>(new[] { "--include-generated" }, Resources.Include_generated_code_files_in_formatting_operations)
{
IsHidden = true
},
Expand All @@ -58,11 +51,66 @@ internal static RootCommand CreateCommandLineOptions()
return rootCommand;
}

internal static bool WasOptionUsed(this ParseResult result, params string[] aliases)
private static LogLevel? ParseLogLevel(ArgumentResult result)
{
var verbosity = result.Tokens.Single();
return verbosity.Value.ToLowerInvariant() switch
{
"q" => LogLevel.Error,
"quiet" => LogLevel.Error,
"m" => LogLevel.Warning,
"minimal" => LogLevel.Warning,
"n" => LogLevel.Information,
"normal" => LogLevel.Information,
"d" => LogLevel.Debug,
"diag" => LogLevel.Debug,
"diagnostic" => LogLevel.Trace,
_ => LogLevel.Information
};
}

private static DiagnosticSeverity? ParseDiagnosticSeverity(ArgumentResult result)
{
return result.Tokens
.Where(token => token.Type == TokenType.Option)
.Any(token => aliases.Contains(token.Value));
if (result.Tokens.Count == 0)
{
// Use Defaults
return DiagnosticSeverity.Error;
}

var severityValue = result.Tokens.Single().Value;
if (!HasMatch(severityValue, out var severity))
{
// Assume that this means our arity is zero and the parser should re-parse accordingly
result.ErrorMessage = $"Cannot specify severity more than once for {result.Argument.Aliases.FirstOrDefault()}";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error message not applicable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this possible since we mark the argument with valid values using FromAmong?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocked by dotnet/command-line-api#955

return default;
}

return severity;

static bool HasMatch(string? severityValue, out DiagnosticSeverity severity)
{
try
{
severity = ParseSeverity(severityValue);
return true;
}
catch (Exception)
{
severity = default;
return false;
}
}

static DiagnosticSeverity ParseSeverity(string? severityValue)
{
return severityValue?.ToLowerInvariant() switch
{
FixSeverity.Error => DiagnosticSeverity.Error,
FixSeverity.Warn => DiagnosticSeverity.Warning,
FixSeverity.Info => DiagnosticSeverity.Info,
_ => throw new ArgumentException(nameof(severityValue)),
};
}
}
}
}
54 changes: 10 additions & 44 deletions src/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.CommandLine;
using System.CommandLine.Invocation;
using System.CommandLine.Parsing;
Expand Down Expand Up @@ -41,9 +42,9 @@ private static async Task<int> Main(string[] args)
public static async Task<int> Run(
string? workspace,
bool folder,
string? fixStyle,
string? fixAnalyzers,
string? verbosity,
DiagnosticSeverity? fixStyle,
DiagnosticSeverity? fixAnalyzers,
LogLevel? verbosity,
bool check,
string[] include,
string[] exclude,
Expand All @@ -57,7 +58,7 @@ public static async Task<int> Run(
}

// Setup logging.
var logLevel = GetLogLevel(verbosity);
var logLevel = verbosity ?? LogLevel.Information;
var logger = SetupLogging(console, logLevel);

// Hook so we can cancel and exit when ctrl+c is pressed.
Expand Down Expand Up @@ -131,10 +132,10 @@ public static async Task<int> Run(
workspacePath,
workspaceType,
logLevel,
fixCodeStyle: s_parseResult.WasOptionUsed("--fix-style", "-fs"),
codeStyleSeverity: GetSeverity(fixStyle ?? FixSeverity.Error),
fixAnalyzers: s_parseResult.WasOptionUsed("--fix-analyzers", "-fa"),
analyerSeverity: GetSeverity(fixAnalyzers ?? FixSeverity.Error),
fixCodeStyle: fixStyle != null,
codeStyleSeverity: fixStyle ?? DiagnosticSeverity.Error,
fixAnalyzers: fixAnalyzers != null,
analyerSeverity: fixAnalyzers ?? DiagnosticSeverity.Error,
saveFormattedFiles: !check,
changesAreErrors: check,
fileMatcher,
Expand All @@ -145,7 +146,7 @@ public static async Task<int> Run(
formatOptions,
logger,
cancellationTokenSource.Token,
createBinaryLog: logLevel == LogLevel.Trace).ConfigureAwait(false);
createBinaryLog: verbosity == LogLevel.Trace).ConfigureAwait(false);

return GetExitCode(formatResult, check);
}
Expand Down Expand Up @@ -177,41 +178,6 @@ internal static int GetExitCode(WorkspaceFormatResult formatResult, bool check)
return formatResult.FilesFormatted == 0 ? 0 : CheckFailedExitCode;
}

internal static LogLevel GetLogLevel(string? verbosity)
{
switch (verbosity)
{
case "q":
case "quiet":
return LogLevel.Error;
case "m":
case "minimal":
return LogLevel.Warning;
case "n":
case "normal":
return LogLevel.Information;
case "d":
case "detailed":
return LogLevel.Debug;
case "diag":
case "diagnostic":
return LogLevel.Trace;
default:
return LogLevel.Information;
}
}

internal static DiagnosticSeverity GetSeverity(string? severity)
{
return severity?.ToLowerInvariant() switch
{
FixSeverity.Error => DiagnosticSeverity.Error,
FixSeverity.Warn => DiagnosticSeverity.Warning,
FixSeverity.Info => DiagnosticSeverity.Info,
_ => throw new ArgumentOutOfRangeException(nameof(severity)),
};
}

private static ILogger<Program> SetupLogging(IConsole console, LogLevel logLevel)
{
var serviceCollection = new ServiceCollection();
Expand Down
9 changes: 6 additions & 3 deletions tests/ProgramTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
using System.CommandLine;
using System.CommandLine.Invocation;
using System.CommandLine.Parsing;

using Microsoft.Extensions.Logging;

using Xunit;

namespace Microsoft.CodeAnalysis.Tools.Tests
Expand Down Expand Up @@ -77,7 +80,7 @@ public void CommandLine_OptionsAreParsedCorrectly()
i1 => Assert.Equal("exclude2", i1));
Assert.True(result.ValueForOption<bool>("check"));
Assert.Equal("report", result.ValueForOption("report"));
Assert.Equal("detailed", result.ValueForOption("verbosity"));
Assert.Equal(LogLevel.Information, result.ValueForOption<LogLevel>("verbosity"));
Assert.True(result.ValueForOption<bool>("include-generated"));
}

Expand Down Expand Up @@ -107,7 +110,7 @@ public void CommandLine_ProjectArgument_WithOption_AfterArgument()
// Assert
Assert.Equal(0, result.Errors.Count);
Assert.Equal("workspaceValue", result.CommandResult.GetArgumentValueOrDefault("workspace"));
Assert.Equal("detailed", result.ValueForOption("verbosity"));
Assert.Equal(LogLevel.Information, result.ValueForOption<LogLevel>("verbosity"));
}

[Fact]
Expand All @@ -122,7 +125,7 @@ public void CommandLine_ProjectArgument_WithOption_BeforeArgument()
// Assert
Assert.Equal(0, result.Errors.Count);
Assert.Equal("workspaceValue", result.CommandResult.GetArgumentValueOrDefault("workspace"));
Assert.Equal("detailed", result.ValueForOption("verbosity"));
Assert.Equal(LogLevel.Information, result.ValueForOption<LogLevel>("verbosity"));
}

[Fact]
Expand Down