-
Notifications
You must be signed in to change notification settings - Fork 686
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.
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
Add why and config commands to dotnet nuget help #5931
Add why and config commands to dotnet nuget help #5931
Conversation
test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatHelpOutputTests.cs
Outdated
Show resolved
Hide resolved
// Assert | ||
var output = consoleOutput.ToString(); | ||
|
||
var commandPattern = @"^\s{2}(\w+)\s{2,}"; // Matches lines starting with two spaces, a word (command), followed by at least two spaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this behavior always same? I mean, will each line have 2 spaces + command + 2 spaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now yes, I know this is not the best way to rely, when I finish the migration to the command I will make sure there is an easier way to know which commands are implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an epic where you are tracking the work for migration? If yes, I suggest adding a tracking issue so that we don't forget.
|
||
foreach (var command in HelpCommands) | ||
{ | ||
Assert.Contains(command, matches); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about what kind of regressions this test will catch, and what it will miss.
It will catch someone removing an existing command from the help. For example, if one command is ported from the existing CLI parser to System.CommandLine and the person removes the comand registration from the dotnet nuget *
code path, this test will fail, telling the developer not to do that.
This test will not catch the scenario where a brand new command is implemeneted and is special cased like config and why are, so they don't get registered through the dotnet nuget *
code path. That's exactly the scenario for config
and why
commands that this PR is fixing.
Since commands don't need to implement an interface, or extend a base class, I don't have any ideas for how we could improve the test to catch the second scenario. I just wanted to raise awareness, in case someone else reviewing has an idea for an easy way to do it. My philosophy for bug fixes is that any tests added should have prevented the bug in the first place, had the test been in place before the bug was introduced. But this test unfortunately would not have, and I can't think of a nice way to do so. Maybe this is just a temporary thing until the System.CommandLine migration is complete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the comparison a little bit, I'll check that both have the same strings in the same order and the sizes are the same.
This could me improved later when we finish the migration because there is no nice way to compare it :(
Bug
Fixes: NuGet/Home#13517
Description
Added missing commands to
dotnet nuget
help commands, since the migration of the commands is still a work in progress we need to add them to the old help option.This PR only register so we can display their description but the functionality is implemented in
System.Commandline.CliCommand
PR Checklist