Fixes #587 Support negative numbers as command option values#732
Fixes #587 Support negative numbers as command option values#732jouniheikniemi wants to merge 2 commits intospectreconsole:mainfrom
Conversation
jouniheikniemi
changed the title
phil-scott-78
left a comment
phil-scott-78
left a comment
There was a problem hiding this comment.
Had some time to look through this. At a glance it looks good. But think we could get some tests that cover the invalid cases as described in the comments of the tokenizer for invalid items?
|
Hey @phil-scott-78, thanks for reviewing. Actually, CommandTreeTokenizer does have (logical) 100 % test coverage already. It is not immediately obvious, as this PR is the first stage that actually adds tests specifically for the Tokenizer; previously, the tokenizer has only been tested through CommandAppTests which test the entire parsing and execution pipeline. However, some of the test scenarios there (say, CommandAppTests.Parsing.InvalidShortOptionName.Should_Return_Correct_Text) do exercise these failure modes. I would personally prefer to have 100 % coverage specifically with the tokenizer test suite, but writing all that is slightly beyond the scope of this issue. Would you expect to see (tokenizer level) tests for the failure scenarios of the ScanShortOptions tokenizer branch or what? TBH, the tokenizer test set in this PR doesn't cover all short option tokenization features (even the happy paths) either, grouped properties probably being the most important missing part. Given that there's no precedent for tokenizer tests in the project, I'm happy to do extra work to set a good example, let's just agree on the goal first. |
FrankRay78
left a comment
FrankRay78
left a comment
There was a problem hiding this comment.
Thank you for submitting this PR, my review comments are largely stylistic to ensure the changes fit stylistically with the rest of the codebase.
| } | ||
| else if (result.Count == 0 && char.IsDigit(current)) | ||
| { | ||
| /* We require short options to be named with letters. Short options that start with a number |
There was a problem hiding this comment.
Everywhere else in the codebase, multi-line comments have simply used the // convention at the beginning of each line. I suggest this comment be updated accordingly to ensure consistency (for better or worse) with the rest of the codebase.
| var represntation = current.ToString(CultureInfo.InvariantCulture); | ||
| var token = new CommandTreeToken(CommandTreeToken.Kind.ShortOption, tokenPosition, represntation, represntation); | ||
| throw CommandParseException.InvalidShortOptionName(reader.Original, token); | ||
| ThrowInvalidShortOptionName(current.ToString(CultureInfo.InvariantCulture)); |
There was a problem hiding this comment.
I do not see the benefit of putting the local three-line logic into a separate private method, which also still references reader.Original from the calling function without passing this in as an explicit argument. I suggest this private method be removed and the logic relocated inline here, instead.
| @@ -0,0 +1,90 @@ | |||
| namespace Spectre.Console.Tests.Unit.Cli; | |||
|
|
|||
| public static class CommandTreeTokenizerTests | |||
There was a problem hiding this comment.
Fantastic that you have introduced explicit unit tests for the tokenizer! A positive move away from the general approach of 'unit tests' being located at the CommandApp level (nb. more like integration tests really).
Does this class need to be static? Unit test classes aren't generally made static - suggest this is also removed to ensure consistency with the rest of the codebase.
| { | ||
| [Fact] | ||
| public void Valueless() | ||
| { |
| var result = CommandTreeTokenizer.Tokenize(new[] { "-a" }); | ||
| Assert.Empty(result.Remaining); | ||
| var t = Assert.Single(result.Tokens); | ||
|
|
| [InlineData("-a=-1.5", null)] | ||
| [InlineData("-a", "-1.5")] | ||
| public void With_Negative_Numeric_Value(string firstArg, string secondArg) | ||
| { |
| { | ||
| args.Add(secondArg); | ||
| } | ||
|
|
| var result = CommandTreeTokenizer.Tokenize(args); | ||
| Assert.Empty(result.Remaining); | ||
| Assert.Equal(2, result.Tokens.Count); | ||
|
|
|
|
||
| [Fact] | ||
| public void With_Negative_Numeric_Prefixed_String_Value() | ||
| { |
| public void With_Negative_Numeric_Prefixed_String_Value() | ||
| { | ||
| var result = CommandTreeTokenizer.Tokenize(new[] { "-6..2 " }); | ||
| Assert.Empty(result.Remaining); |
This is a significant improvement that @jouniheikniemi has introduced and I would be keen to see this PR, once review comments are addressed 😊, merged into the codebase. Particularly since I have a raft of CommandTreeTokenizer tests I would like to add myself, ideally into the same class being introduced in this PR. (given this PR has been languishing since Feb, I'm happy to fork Jouni's branch, bring it up to date, and make the review comments myself to get this over the line, if asked...) |
|
This should be closed as it's been fully included in #1048 [I don't have permission to close another person's PR] |
|
I was about to get back to your review comments, but it seems you'd rather pick up the ball yourself; fine with me. Closing the PR. Thanks! |
|
I was keen to merge your excellent work @jouniheikniemi - thanks for your contribution. |
3 participants
Decided to create some unit tests for the tokenizer, as it would've been too much of a mess to test all of this through CommandApp. That's obviously up for debate, but at least the tokenizer tests helped me with this.