Skip to content
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

Jump to bottom

Add parsed unknown flag to remaining arguments for a branch with a default command #1660

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add parsed unknown flag to remaining arguments for a branch with a default command #1660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b60cff7
New unit tests to ensure unknown flags are added to remaining args
FrankRay78 Oct 4, 2024
332bea1
Cosmetic: Remove ugly looking variable naming
FrankRay78 Oct 4, 2024
04208d8
Add strict parsing to almost all branches tests, an obvious omission
FrankRay78 Oct 4, 2024
c33a4cf
Fix tests with incorrect validation that were in the main branch
FrankRay78 Oct 4, 2024
e69888a
Significant improvement to the command line parsing
FrankRay78 Oct 4, 2024
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
5 changes: 5 additions & 0 deletions src/Spectre.Console.Cli/CommandParseException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,4 +113,9 @@ internal static CommandParseException ValueIsNotInValidFormat(string value)
var text = $"[red]Error:[/] The value '[white]{value}[/]' is not in a correct format";
return new CommandParseException("Could not parse value", new Markup(text));
}

internal static CommandParseException UnknownParsingError()
{
return new CommandParseException("An unknown error occured when parsing the arguments.");
}
}
95 changes: 81 additions & 14 deletions src/Spectre.Console.Cli/Internal/CommandExecutor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
using static Spectre.Console.Cli.CommandTreeTokenizer;

namespace Spectre.Console.Cli;

internal sealed class CommandExecutor
Expand Down Expand Up @@ -101,34 +103,99 @@ public async Task<int> Execute(IConfiguration configuration, IEnumerable<string>
}
}

[SuppressMessage("StyleCop.CSharp.LayoutRules", "SA1513:Closing brace should be followed by blank line", Justification = "Improves code readability by grouping together related statements into a block")]
private CommandTreeParserResult ParseCommandLineArguments(CommandModel model, CommandAppSettings settings, IReadOnlyList<string> args)
{
var parser = new CommandTreeParser(model, settings.CaseSensitivity, settings.ParsingMode, settings.ConvertFlagsToRemainingArguments);
CommandTreeParserResult? parsedResult = null;
CommandTreeTokenizerResult tokenizerResult;

var parserContext = new CommandTreeParserContext(args, settings.ParsingMode);
var tokenizerResult = CommandTreeTokenizer.Tokenize(args);
var parsedResult = parser.Parse(parserContext, tokenizerResult);
try
{
(parsedResult, tokenizerResult) = InternalParseCommandLineArguments(model, settings, args);

var lastParsedLeaf = parsedResult.Tree?.GetLeafCommand();
var lastParsedCommand = lastParsedLeaf?.Command;
if (lastParsedLeaf != null && lastParsedCommand != null &&
var lastParsedLeaf = parsedResult.Tree?.GetLeafCommand();
var lastParsedCommand = lastParsedLeaf?.Command;

if (lastParsedLeaf != null && lastParsedCommand != null &&
lastParsedCommand.IsBranch && !lastParsedLeaf.ShowHelp &&
lastParsedCommand.DefaultCommand != null)
{
// Adjust for any parsed remaining arguments by
// inserting the the default command ahead of them.
var position = tokenizerResult.Tokens.Position;
foreach (var parsedRemaining in parsedResult.Remaining.Parsed)
{
position--;
position -= parsedRemaining.Count(value => value != null);
}
position = position < 0 ? 0 : position;

// Insert this branch's default command into the command line
// arguments and try again to see if it will parse.
var argsWithDefaultCommand = new List<string>(args);
argsWithDefaultCommand.Insert(position, lastParsedCommand.DefaultCommand.Name);

(parsedResult, tokenizerResult) = InternalParseCommandLineArguments(model, settings, argsWithDefaultCommand);
}
}
catch (CommandParseException) when (parsedResult == null && settings.ParsingMode == ParsingMode.Strict)
{
// Insert this branch's default command into the command line
// arguments and try again to see if it will parse.
var argsWithDefaultCommand = new List<string>(args);
// The parsing exception might be resolved by adding in the default command,
// but we can't know for sure. Take a brute force approach and try this for
// every position between the arguments.
for (int i = 0; i < args.Count; i++)
{
var argsWithDefaultCommand = new List<string>(args);
argsWithDefaultCommand.Insert(args.Count - i, "__default_command");

try
{
(parsedResult, tokenizerResult) = InternalParseCommandLineArguments(model, settings, argsWithDefaultCommand);

argsWithDefaultCommand.Insert(tokenizerResult.Tokens.Position, lastParsedCommand.DefaultCommand.Name);
break;
}
catch (CommandParseException)
{
// Continue.
}
}

parserContext = new CommandTreeParserContext(argsWithDefaultCommand, settings.ParsingMode);
tokenizerResult = CommandTreeTokenizer.Tokenize(argsWithDefaultCommand);
parsedResult = parser.Parse(parserContext, tokenizerResult);
if (parsedResult == null)
{
// Failed to parse having inserted the default command between each argument.
// Repeat the parsing of the original arguments to throw the correct exception.
InternalParseCommandLineArguments(model, settings, args);
}
}

if (parsedResult == null)
{
// The arguments failed to parse despite everything we tried above.
// Exceptions should be thrown above before ever getting this far,
// however the following is the ulimately backstop and avoids
// the compiler from complaining about returning null.
throw CommandParseException.UnknownParsingError();
}

return parsedResult;
}

/// <summary>
/// Parse the command line arguments using the specified <see cref="CommandModel"/> and <see cref="CommandAppSettings"/>,
/// returning the parser and tokenizer results.
/// </summary>
/// <returns>The parser and tokenizer results as a tuple.</returns>
private (CommandTreeParserResult ParserResult, CommandTreeTokenizerResult TokenizerResult) InternalParseCommandLineArguments(CommandModel model, CommandAppSettings settings, IReadOnlyList<string> args)
{
var parser = new CommandTreeParser(model, settings.CaseSensitivity, settings.ParsingMode, settings.ConvertFlagsToRemainingArguments);

var parserContext = new CommandTreeParserContext(args, settings.ParsingMode);
var tokenizerResult = CommandTreeTokenizer.Tokenize(args);
var parsedResult = parser.Parse(parserContext, tokenizerResult);

return (parsedResult, tokenizerResult);
}

private static async Task<int> Execute(
CommandTree leaf,
CommandTree tree,
Expand Down
106 changes: 68 additions & 38 deletions src/Tests/Spectre.Console.Cli.Tests/Unit/CommandAppTests.Branches.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,16 @@ public sealed partial class CommandAppTests
{
public sealed class Branches
{
[Fact]
public void Should_Run_The_Default_Command_On_Branch()
[Theory]
[InlineData(true)]
[InlineData(false)]
public void Should_Run_The_Default_Command_On_Branch(bool strictParsing)
{
// Given
var app = new CommandAppTester();
app.Configure(config =>
{
config.Settings.StrictParsing = strictParsing;
config.PropagateExceptions();
config.AddBranch<AnimalSettings>("animal", animal =>
{
Expand All @@ -29,13 +32,16 @@ public void Should_Run_The_Default_Command_On_Branch()
result.Settings.ShouldBeOfType<CatSettings>();
}

[Fact]
public void Should_Throw_When_No_Default_Command_On_Branch()
[Theory]
[InlineData(true)]
[InlineData(false)]
public void Should_Throw_When_No_Default_Command_On_Branch(bool strictParsing)
{
// Given
var app = new CommandAppTester();
app.Configure(config =>
{
config.Settings.StrictParsing = strictParsing;
config.PropagateExceptions();
config.AddBranch<AnimalSettings>("animal", animal => { });
});
Expand All @@ -56,10 +62,8 @@ public void Should_Throw_When_No_Default_Command_On_Branch()
});
}

[SuppressMessage("StyleCop.CSharp.LayoutRules", "SA1515:SingleLineCommentMustBePrecededByBlankLine", Justification = "Helps to illustrate the expected behaviour of this unit test.")]
[SuppressMessage("StyleCop.CSharp.SpacingRules", "SA1005:SingleLineCommentsMustBeginWithSingleSpace", Justification = "Helps to illustrate the expected behaviour of this unit test.")]
[Fact]
public void Should_Be_Unable_To_Parse_Default_Command_Arguments_Relaxed_Parsing()
public void Should_Parse_Branch_And_Default_Command_Arguments_Relaxed_Parsing()
{
// Given
var app = new CommandAppTester();
Expand All @@ -75,25 +79,27 @@ public void Should_Be_Unable_To_Parse_Default_Command_Arguments_Relaxed_Parsing(
// When
var result = app.Run(new[]
{
// The CommandTreeParser should be unable to determine which command line
// arguments belong to the branch and which belong to the branch's
// default command (once inserted).
"animal", "4", "--name", "Kitty",
// The CommandTreeParser should determine which command line arguments
// belong to the branch and which belong to the branch's default command.
"animal", "4", "-a", "false", "--name", "Kitty", "--agility", "four", "--nick-name", "Felix"
});

// Then
result.ExitCode.ShouldBe(0);
result.Settings.ShouldBeOfType<CatSettings>().And(cat =>
{
cat.IsAlive.ShouldBeFalse();
cat.Legs.ShouldBe(4);
//cat.Name.ShouldBe("Kitty"); //<-- Should normally be correct, but instead name will be added to the remaining arguments (see below).
cat.Name.ShouldBe("Kitty");
cat.Agility.ShouldBe(4);
});
result.Context.Remaining.Parsed.Count.ShouldBe(1);
result.Context.ShouldHaveRemainingArgument("--name", values: new[] { "Kitty", });
result.Context.ShouldHaveRemainingArgument("--nick-name", values: new[] { "Felix" });
result.Context.Remaining.Raw.Count.ShouldBe(0);
}

[Fact]
public void Should_Be_Unable_To_Parse_Default_Command_Arguments_Strict_Parsing()
public void Should_Parse_Branch_And_Default_Command_Arguments_Strict_Parsing()
{
// Given
var app = new CommandAppTester();
Expand All @@ -108,31 +114,37 @@ public void Should_Be_Unable_To_Parse_Default_Command_Arguments_Strict_Parsing()
});

// When
var result = Record.Exception(() =>
var result = app.Run(new[]
{
app.Run(new[]
{
// The CommandTreeParser should be unable to determine which command line
// arguments belong to the branch and which belong to the branch's
// default command (once inserted).
"animal", "4", "--name", "Kitty",
});
// The CommandTreeParser should determine which command line arguments
// belong to the branch and which belong to the branch's default command.
"animal", "4", "-a", "false", "--name", "Kitty", "--agility", "four", "--", "--nick-name", "Felix"
});

// Then
result.ShouldBeOfType<CommandParseException>().And(ex =>
result.ExitCode.ShouldBe(0);
result.Settings.ShouldBeOfType<CatSettings>().And(cat =>
{
ex.Message.ShouldBe("Unknown option 'name'.");
cat.IsAlive.ShouldBeFalse();
cat.Legs.ShouldBe(4);
cat.Name.ShouldBe("Kitty");
cat.Agility.ShouldBe(4);
});
result.Context.Remaining.Parsed.Count.ShouldBe(1);
result.Context.ShouldHaveRemainingArgument("--nick-name", values: new[] { "Felix" });
result.Context.Remaining.Raw.Count.ShouldBe(2);
}

[Fact]
public void Should_Run_The_Default_Command_On_Branch_On_Branch()
[Theory]
[InlineData(true)]
[InlineData(false)]
public void Should_Run_The_Default_Command_On_Branch_On_Branch(bool strictParsing)
{
// Given
var app = new CommandAppTester();
app.Configure(config =>
{
config.Settings.StrictParsing = strictParsing;
config.PropagateExceptions();
config.AddBranch<AnimalSettings>("animal", animal =>
{
Expand All @@ -154,13 +166,16 @@ public void Should_Run_The_Default_Command_On_Branch_On_Branch()
result.Settings.ShouldBeOfType<CatSettings>();
}

[Fact]
public void Should_Run_The_Default_Command_On_Branch_On_Branch_With_Arguments()
[Theory]
[InlineData(true)]
[InlineData(false)]
public void Should_Run_The_Default_Command_On_Branch_On_Branch_With_Arguments(bool strictParsing)
{
// Given
var app = new CommandAppTester();
app.Configure(config =>
{
config.Settings.StrictParsing = strictParsing;
config.PropagateExceptions();
config.AddBranch<AnimalSettings>("animal", animal =>
{
Expand All @@ -186,13 +201,16 @@ public void Should_Run_The_Default_Command_On_Branch_On_Branch_With_Arguments()
});
}

[Fact]
public void Should_Run_The_Default_Command_Not_The_Named_Command_On_Branch()
[Theory]
[InlineData(true)]
[InlineData(false)]
public void Should_Run_The_Default_Command_Not_The_Named_Command_On_Branch(bool strictParsing)
{
// Given
var app = new CommandAppTester();
app.Configure(config =>
{
config.Settings.StrictParsing = strictParsing;
config.PropagateExceptions();
config.AddBranch<AnimalSettings>("animal", animal =>
{
Expand All @@ -213,13 +231,16 @@ public void Should_Run_The_Default_Command_Not_The_Named_Command_On_Branch()
result.Settings.ShouldBeOfType<CatSettings>();
}

[Fact]
public void Should_Run_The_Named_Command_Not_The_Default_Command_On_Branch()
[Theory]
[InlineData(true)]
[InlineData(false)]
public void Should_Run_The_Named_Command_Not_The_Default_Command_On_Branch(bool strictParsing)
{
// Given
var app = new CommandAppTester();
app.Configure(config =>
{
config.Settings.StrictParsing = strictParsing;
config.PropagateExceptions();
config.AddBranch<AnimalSettings>("animal", animal =>
{
Expand All @@ -246,13 +267,16 @@ public void Should_Run_The_Named_Command_Not_The_Default_Command_On_Branch()
});
}

[Fact]
public void Should_Allow_Multiple_Branches_Multiple_Commands()
[Theory]
[InlineData(true)]
[InlineData(false)]
public void Should_Allow_Multiple_Branches_Multiple_Commands(bool strictParsing)
{
// Given
var app = new CommandAppTester();
app.Configure(config =>
{
config.Settings.StrictParsing = strictParsing;
config.PropagateExceptions();
config.AddBranch<AnimalSettings>("animal", animal =>
{
Expand Down Expand Up @@ -282,13 +306,16 @@ public void Should_Allow_Multiple_Branches_Multiple_Commands()
});
}

[Fact]
public void Should_Allow_Single_Branch_Multiple_Commands()
[Theory]
[InlineData(true)]
[InlineData(false)]
public void Should_Allow_Single_Branch_Multiple_Commands(bool strictParsing)
{
// Given
var app = new CommandAppTester();
app.Configure(config =>
{
config.Settings.StrictParsing = strictParsing;
config.PropagateExceptions();
config.AddBranch<AnimalSettings>("animal", animal =>
{
Expand All @@ -315,13 +342,16 @@ public void Should_Allow_Single_Branch_Multiple_Commands()
});
}

[Fact]
public void Should_Allow_Single_Branch_Single_Command()
[Theory]
[InlineData(true)]
[InlineData(false)]
public void Should_Allow_Single_Branch_Single_Command(bool strictParsing)
{
// Given
var app = new CommandAppTester();
app.Configure(config =>
{
config.Settings.StrictParsing = strictParsing;
config.PropagateExceptions();
config.AddBranch<AnimalSettings>("animal", animal =>
{
Expand Down
Loading