Apply help to all symbols in the CLI declaration #2442

hasnik · 2024-06-13T09:54:11Z

This PR aims to address #2343 by adding help options to alll comands in the tree and includes tests.

Would love any thoughts or feedback as this is my first open-source PR.

hasnik · 2024-06-13T09:54:43Z

@dotnet-policy-service agree

hasnik · 2024-06-13T10:10:52Z

src/System.CommandLine.Subsystems/HelpSubsystem.cs

+        static void AddOptionRecursively(CliCommand command, CliOption option)
+        {
+            command.Add(option);
+
+            foreach (var subcommand in command.Subcommands)
+            {
+                AddOptionRecursively(subcommand, option);
+            }
+        }


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.

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 a CliCommand.

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.

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!

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.

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.

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 a CliCommand.

Looking around on main-powderhouse I can see there's a GetSymbolByName in ParseResult class, but unfrotunately not available on CliCommand. Are you planning on bringing it or should I try to access it with some workaround?

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.

Haven't really thought about it before, but I'd say your first intuition makes a lot of sense to me.

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).

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 :)

JeanJoeris

I think this approach makes sense, thanks for submitting the PR! I left one clarifying question on one of the tests.

JeanJoeris · 2024-06-18T15:13:27Z

src/System.CommandLine.Subsystems.Tests/HelpSubsystemTests.cs

+        [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!***");


I might be misssing something but it seems like Custom_help_subsystem_can_be_used and Custom_help_subsystem_can _replace_standard are the same test. Is this the case? If so I think we might want to consolidate them

I've copied these shamelessly from VersionSubsystemTests.cs

Couldn't really tell why there are two tests with identical body there, but I assumed I don't understand something and chose to add it. Removing it in the upcoming commit. Should I also remove the duplicate tests from VersionSubsystemTests.cs - specifically Custom_version_subsystem_can_replace_standard?

KathleenDollard

Welcome! Thanks for this and including great tests!

In the long comment, this opens a couple of design questions and summer absences may result in a delay, but we should definitely be able to take this with tweaks.

src/System.CommandLine.Subsystems.Tests/TestData.cs

KathleenDollard · 2024-06-18T15:30:32Z

src/System.CommandLine.Subsystems/HelpSubsystem.cs

+        static void AddOptionRecursively(CliCommand command, CliOption option)
+        {
+            command.Add(option);
+
+            foreach (var subcommand in command.Subcommands)
+            {
+                AddOptionRecursively(subcommand, option);
+            }
+        }


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 a CliCommand.

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.

KathleenDollard · 2024-06-18T15:32:04Z

src/System.CommandLine.Subsystems.Tests/HelpSubsystemTests.cs

+            var result = pipeline.Parse(configuration, "");
+
+            rootCommand.Options.Should().NotBeNull();
+            rootCommand.Options


We have a SymbolByName method in flight and need to ensure that it will work in this context.

Good to know! Will update the test when it becomes available on the CliCommand or should I try to grab the help option from ParseResult?

src/System.CommandLine.Subsystems.Tests/HelpSubsystemTests.cs

src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs

Value declared as non-null Co-authored-by: Kevin B <[email protected]>

Co-authored-by: Kevin B <[email protected]>

Value declared as non-null Co-authored-by: Kevin B <[email protected]>

Co-authored-by: Kevin B <[email protected]>

Michał Haśnik added 2 commits June 13, 2024 11:43

Apply help to all symbols in the CLI

203c023

Add HelpSubsystem tests

fb6952a

Removed code comment

2408173

hasnik commented Jun 13, 2024

View reviewed changes

KathleenDollard added the Powderhouse Work to isolate parser and features label Jun 17, 2024

JeanJoeris reviewed Jun 18, 2024

View reviewed changes

KathleenDollard reviewed Jun 18, 2024

View reviewed changes

Keboo reviewed Jun 18, 2024

View reviewed changes

hasnik and others added 8 commits June 18, 2024 19:09

Removed unnecessary not null check

63ae37a

Value declared as non-null Co-authored-by: Kevin B <[email protected]>

Removed unnecessary string interpolation on constant string value

577c60d

Co-authored-by: Kevin B <[email protected]>

Removed unnecessary not null check

f4a627a

Value declared as non-null Co-authored-by: Kevin B <[email protected]>

Removed unnecessary string interpolation on constant string value

141a74a

Co-authored-by: Kevin B <[email protected]>

Removed unnecessary string interpolation on constant string value

93fb212

Co-authored-by: Kevin B <[email protected]>

Changed namespace declaration to file scoped

910aa7e

Consolided Help subsystem tests with same body

e028d1a

Removed mistakenly copied comment

9a48fef

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply help to all symbols in the CLI declaration #2442

Apply help to all symbols in the CLI declaration #2442

hasnik commented Jun 13, 2024

hasnik commented Jun 13, 2024

hasnik Jun 13, 2024

KathleenDollard Jun 18, 2024

hasnik Jun 18, 2024 •

edited

Loading

JeanJoeris left a comment

JeanJoeris Jun 18, 2024 •

edited

Loading

hasnik Jun 18, 2024

KathleenDollard left a comment

KathleenDollard Jun 18, 2024

KathleenDollard Jun 18, 2024

hasnik Jun 18, 2024 •

edited

Loading

Apply help to all symbols in the CLI declaration #2442

Are you sure you want to change the base?

Apply help to all symbols in the CLI declaration #2442

Conversation

hasnik commented Jun 13, 2024

hasnik commented Jun 13, 2024

hasnik Jun 13, 2024

Choose a reason for hiding this comment

KathleenDollard Jun 18, 2024

Choose a reason for hiding this comment

hasnik Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

JeanJoeris left a comment

Choose a reason for hiding this comment

JeanJoeris Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

hasnik Jun 18, 2024

Choose a reason for hiding this comment

KathleenDollard left a comment

Choose a reason for hiding this comment

KathleenDollard Jun 18, 2024

Choose a reason for hiding this comment

KathleenDollard Jun 18, 2024

Choose a reason for hiding this comment

hasnik Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

hasnik Jun 18, 2024 •

edited

Loading

JeanJoeris Jun 18, 2024 •

edited

Loading

hasnik Jun 18, 2024 •

edited

Loading