dotmake-build / command-line

Declarative syntax for System.CommandLine via attributes for easy, fast, strongly-typed (no reflection) usage. Includes a source generator which automagically converts your classes to CLI commands and properties to CLI options or CLI arguments.
https://dotmake.build
MIT License
79 stars 6 forks source link

Proposal for fix for #14 by adding support for injecting parent commands as properties. #15

Closed poizan42 closed 8 months ago

poizan42 commented 8 months ago

Adds support for injecting parent commands into child commands by adding properties.

This implementation adds a CliBindContext class for use when binding which caches created command class instance so they are only created at most once.

Usage example ```CSharp // Sub-commands can get a reference to the parent command by adding a property of the parent command type. [CliCommand(Description = "A root cli command with nested children")] public class RootWithNestedChildrenReferencingRootCliCommand { [CliOption(Description = "This is a global option (Recursive option on the root command), it can appear anywhere on the command line", Recursive = true)] public string GlobalOption1 { get; set; } = "DefaultForGlobalOption1"; [CliArgument(Description = "Description for RootArgument1")] public string RootArgument1 { get; set; } public void Run(CliContext context) { context.ShowValues(); } [CliCommand(Description = "A nested level 1 sub-command which accesses the root command")] public class Level1SubCliCommand { [CliOption(Description = "This is global for all sub commands (it can appear anywhere after the level-1 verb)", Recursive = true)] public string Level1RecursiveOption1 { get; set; } = "DefaultForLevel1RecusiveOption1"; [CliArgument(Description = "Description for Argument1")] public string Argument1 { get; set; } // The parent command gets automatically injected public RootWithNestedChildrenReferencingRootCliCommand RootCommand { get; set; } public void Run(CliContext context) { context.ShowValues(); } [CliCommand(Description = "A nested level 2 sub-command which accesses its parent commands")] public class Level2SubCliCommand { [CliOption(Description = "Description for Option1")] public string Option1 { get; set; } = "DefaultForOption1"; [CliArgument(Description = "Description for Argument1")] public string Argument1 { get; set; } // All ancestor commands gets injected public RootWithNestedChildrenReferencingRootCliCommand RootCommand { get; set; } public Level1SubCliCommand ParentCommand { get; set; } public void Run(CliContext context) { context.ShowValues(); Console.WriteLine($"Level1RecursiveOption1 = {ParentCommand.Level1RecursiveOption1}"); Console.WriteLine($"parent Argument1 = {ParentCommand.Argument1}"); Console.WriteLine($"GlobalOption1 = {RootCommand.GlobalOption1}"); Console.WriteLine($"RootArgument1 = {RootCommand.RootArgument1}"); } } } } ```
calacayir commented 8 months ago

Thanks but I have some concerns:

I will let you know soon with the results.

poizan42 commented 8 months ago
  • Do we really need to access parent commands other than accessing recursive options. I guess no so CliParentCommandRefInfo should be replaced with something like CliRecursiveOptionInfo. That way we can specify only the property with same name for recursive option. The parent command accessor may also be supported if really required though.

How would you access ParentCommand.Argument1 and RootCommand.RootArgument1 in my example otherwise? These are just required arguments to the parent verbs and neither recursive nor options.

E.g. if you run my example with the command line

    > ./TestApp Foo level-1 Bar level-2 Baz

You get the output

Command = "level-2" [Sub-command]
Argument 'argument-1' = "Baz"
Option '--option-1' = "DefaultForOption1"
Level1RecursiveOption1 = DefaultForLevel1RecusiveOption1
parent Argument1 = Bar
GlobalOption1 = DefaultForGlobalOption1
RootArgument1 = Foo

Without this your only way of getting the "Foo" and "Bar" arguments would be to use Bind() or fetching them out of the ParseResult manually.

calacayir commented 8 months ago

Ok I didn't pay attention to the arguments in the example. Then your implementation is fine and logical. I will just tidy some parts.

poizan42 commented 8 months ago
  • I think we don't need another context class as CliBindContext, but what you did in BindOrGetBindResult is a good idea and I think we need to incorporate that into ParseResult.Bind extension method.

On that note, I don't think there is any sensible place we can store the constructed command class instances on ParseResult itself. It's not sealed so I guess we could create a derived class, but that is hardly how it's meant to be used. We could ofc. store them in a ConditionalWeakTable, but that seems pretty hackish. Also FWIW the CliBindContext gives users the freedom to choose whether they want a new set of command class instances or get the ones already constructed.

calacayir commented 8 months ago

Yes I noticed that. For now, I used a ConcurrentDictionary to cache bind results per ParseResult. Check v1.8.6 and let me know.