-
Notifications
You must be signed in to change notification settings - Fork 386
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
Apply help to all symbols in the CLI declaration #2442
Open
hasnik
wants to merge
11
commits into
dotnet:main-powderhouse
Choose a base branch
from
hasnik:mh/apply-help-to-all-symbols-in-the-CLI
base: main-powderhouse
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+117
−2
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
203c023
Apply help to all symbols in the CLI
fb6952a
Add HelpSubsystem tests
2408173
Removed code comment
63ae37a
Removed unnecessary not null check
hasnik 577c60d
Removed unnecessary string interpolation on constant string value
hasnik f4a627a
Removed unnecessary not null check
hasnik 141a74a
Removed unnecessary string interpolation on constant string value
hasnik 93fb212
Removed unnecessary string interpolation on constant string value
hasnik 910aa7e
Changed namespace declaration to file scoped
e028d1a
Consolided Help subsystem tests with same body
9a48fef
Removed mistakenly copied comment
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
75 changes: 75 additions & 0 deletions
75
src/System.CommandLine.Subsystems.Tests/HelpSubsystemTests.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
// Copyright (c) .NET Foundation and contributors. All rights reserved. | ||
// Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
||
using FluentAssertions; | ||
using Xunit; | ||
using System.CommandLine.Parsing; | ||
|
||
namespace System.CommandLine.Subsystems.Tests; | ||
|
||
public class HelpSubsystemTests | ||
{ | ||
[Fact] | ||
public void When_help_subsystem_is_used_the_help_option_is_added_to_each_command_in_the_tree() | ||
{ | ||
var rootCommand = new CliRootCommand | ||
{ | ||
new CliOption<bool>("-x"), // add option that is expected for the test data used here | ||
new CliCommand("a") | ||
}; | ||
var configuration = new CliConfiguration(rootCommand); | ||
|
||
var pipeline = Pipeline.CreateEmpty(); | ||
pipeline.Help = new HelpSubsystem(); | ||
|
||
// Parse is used because directly calling Initialize would be unusual | ||
var result = pipeline.Parse(configuration, ""); | ||
|
||
rootCommand.Options | ||
.Count(x => x.Name == "--help") | ||
.Should() | ||
.Be(1); | ||
var subcommand = rootCommand.Subcommands.First(); | ||
subcommand.Options | ||
.Count(x => x.Name == "--help") | ||
.Should() | ||
.Be(1); | ||
} | ||
|
||
[Theory] | ||
[ClassData(typeof(TestData.Help))] | ||
public void Help_is_activated_only_when_requested(string input, bool result) | ||
{ | ||
CliRootCommand rootCommand = [new CliOption<bool>("-x")]; // add random option as empty CLIs are rare | ||
var configuration = new CliConfiguration(rootCommand); | ||
var helpSubsystem = new HelpSubsystem(); | ||
var args = CliParser.SplitCommandLine(input).ToList().AsReadOnly(); | ||
|
||
Subsystem.Initialize(helpSubsystem, configuration, args); | ||
|
||
var parseResult = CliParser.Parse(rootCommand, input, configuration); | ||
var isActive = Subsystem.GetIsActivated(helpSubsystem, parseResult); | ||
|
||
isActive.Should().Be(result); | ||
} | ||
|
||
[Fact] | ||
public void Outputs_help_message() | ||
{ | ||
var consoleHack = new ConsoleHack().RedirectToBuffer(true); | ||
var helpSubsystem = new HelpSubsystem(); | ||
Subsystem.Execute(helpSubsystem, new PipelineResult(null, "", null, consoleHack)); | ||
consoleHack.GetBuffer().Trim().Should().Be("Help me!"); | ||
} | ||
|
||
[Fact] | ||
public void Custom_help_subsystem_can_be_used() | ||
{ | ||
var consoleHack = new ConsoleHack().RedirectToBuffer(true); | ||
var pipeline = Pipeline.CreateEmpty(); | ||
pipeline.Help = new AlternateSubsystems.AlternateHelp(); | ||
|
||
pipeline.Execute(new CliConfiguration(new CliRootCommand()), "-h", consoleHack); | ||
consoleHack.GetBuffer().Trim().Should().Be("***Help me!***"); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 imagine this method should belong to CliCommand or some extensions method, but I didn't want to change too much in this PR without consulting you first.
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 am torn between having this be an extension method on CliOption or CliCommand. Regardless, I think it belongs in the core (System.CommandLine) and appreciate you being conservative on this. Let's get some more thoughts on the location.
I think we should do a check so that if it is added twice we don't have problems. I think we should do that by name. We are in process of adding a
GetSymbolName
method, although I need to check that it is available on aCliCommand
.We will need to decide if first or last should win. My first intuition is to have the first win, so calling this method is "add the option to any commands that do not already have an option by this name". If you wanted custom help on one option (or another option), I think it would be most natural to add that first. Similarly, if you wanted one branch of commands in a complex tree to do help differently (
dotnet new
for example), you'd just have to call in order most specialized to least.The other thing nagging on this is that our previous approach is that the CLI tree needs to be created before this is called. Another approach (although I like it less) is to mark the command and add the option to each command immediately before parsing. I am concerned about bad behavior if the tree is reused and a different help system is set (unless we copy the tree).
Sorry for not having a clear answer today on this. It's summer and we have some folks out, so it may be a bit before we have a clear answer on where and how to do the check.
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 happy to let it wait for the right time and update the code whenever the questions you pointed out have answers. Thank you sharing all the context, I do appreciate it very much!
My personal taste would be
CliCommand
, but I'm unsure as I know my pov is most likely restricted to my use-cases and understand that every decision impacts a huge audience.Looking around on
main-powderhouse
I can see there's aGetSymbolByName
inParseResult
class, but unfrotunately not available onCliCommand
. Are you planning on bringing it or should I try to access it with some workaround?Haven't really thought about it before, but I'd say your first intuition makes a lot of sense to me.
I believe there's much smarter individuals to discuss this with, but my intuition would be to stick with 'frozen' CLI tree - it just sounds right, unless there's any good reason not to use it. Is there any appropriate plugging point to add help 'higher' in the chain? Whatever the decision will be, do let me know - I'd love to have my small addition in the .NET ecosystem :)