spectre.console
spectre.console copied to clipboard
spectreconsole
Fixes #587 Support negative numbers as command option values
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.
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.
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
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 https://github.com/spectreconsole/spectre.console/pull/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.