GH-959: Invalid handling of parameter values with quotes (double and single, enclosed and open), whitespace, hyphens and special characters (eg. emojis) #1036
Conversation
…incorrectly/unnecessarily manipulated by spectreconsole
…tes, hyphens and spaces by the CommandTreeTokenizer
… spaces/hyphens/quotes
| { | ||
| [Fact] | ||
| [Expectation("Test_1")] | ||
| public Task Should_Return_Correct_Text() |
There was a problem hiding this comment.
Would prefer if we kept this test to ensure backwards compatibility.
There was a problem hiding this comment.
The unit test, UnterminatedQuote.Should_Return_Correct_Text() throws an error when a terminating quote isn't found when parsing a command line argument and converting it into (one or more) tokens.
I deleted the test because CommandTreeTokenizer now handles both terminated and unterminated quotes when received in an argument.
CommandTreeTokenizerTests evidences this behaviour, see the various tests with the following decorations:
// Double-quote handling
[InlineData("\"")]
[InlineData("\"\"")]
[InlineData("\"Rufus\"")]
[InlineData("\" Rufus\"")]
[InlineData("\"-R\"")]
[InlineData("\"-Rufus\"")]
[InlineData("\" -Rufus\"")]
// Single-quote handling
[InlineData("'")]
[InlineData("''")]
[InlineData("'Rufus'")]
[InlineData("' Rufus'")]
[InlineData("'-R'")]
[InlineData("'-Rufus'")]
[InlineData("' -Rufus'")]
[InlineData(" \" -Rufus -- ")]
[InlineData("Name=\" -Rufus --' ")]
[nb. ScanQuotedString(...) was also removed in this PR as the ScanString can now handle quotes without special consideration]
|
|
||
| [Fact] | ||
| [Expectation("Quoted_Strings")] | ||
| public Task Should_Parse_Quoted_Strings_Correctly() |
There was a problem hiding this comment.
Would prefer if we kept this test to ensure backwards compatibility.
There was a problem hiding this comment.
I can add it back, that's not a problem.
However, let me explain why I re/moved this first, see if that makes a difference...
The same [functionally equivalent] test is now located in CommandAppTests+Remaining (Spectre.Console.Tests.Unit.Cli), namely public void Should_Preserve_Quotes_Hyphen_Delimiters():
var result = app.Run(new[]
{
"animal", "4", "dog", "12", "--",
"/c", "\"set && pause\"",
"Name=\" -Rufus --' ",
});
// Then
result.Context.Remaining.Raw.Count.ShouldBe(3);
result.Context.Remaining.Raw[0].ShouldBe("/c");
result.Context.Remaining.Raw[1].ShouldBe("\"set && pause\"");
I moved it here as every other test in test/Spectre.Console.Cli.Tests/Unit/CommandAppTests.Parsing.cs is actually handling error states, not happy paths. Plus it seemed an appropriate new location alongside the other dedicated remaining args unit tests.
Also, and perhaps most importantly, by re/moving the Should_Parse_Quoted_Strings_Correctly() test out of CommandAppTests.Parsing, I felt it paved the way to rename the whole class (perhaps more appropriately) to CommandAppTests.ParsingErrors.
[nb. now that I've written the above, perhaps the renaming should happen as I suggest, but with CommandAppTests.Parsing also being resurrected with a single method, the public Task Should_Parse_Quoted_Strings_Correctly() to ensure backwards compatibility as you suggest]
Happy to do any of the above, if you let me know which one. (sorry if I'm making things overly complicated...)
…eter-values-starting-with-double-quotes
|
Closing as superseded by #1048 |
This PR fully implements #959, #842, #890, namely POSIX compliant handling of quotes (double and single, enclosed and open), whitespace, hyphens and special characters (eg. emojis) when arriving in the
IEnumerable<string> argsarray, agnostic of the syntax required by the surrounding shell to do so.A significant increase to unit test coverage accompanies this PR, particularly focused on the
CommandTreeTokenizerclass. 41 new unit tests cover the parsing and tokenisation of string, long and short options. Minimal changes to existing tests were necessary, and only when the existing implementation incorrectly (according to POSIX) handled quotes/whitespace/hyphens etc.When this PR is considered collectively with #1029 and #732, these PR's would make a significant enhancement to command line parsing, extending behaviour across all platforms (behaviour already familiar to Mac and Linux users), and IMHO, would make an excellent minor release.