JasperFx/oakton

Optional arguments broken in 2.0

kae opened this issue · 5 comments

kae commented

afc4f12#diff-1653881f4444af532bfaf6787fb0c0f4

This commit broke possibility of optional arguments

-            var command = Build(commandName);
+            var usages = new UsageGraph(_commandTypes[commandName]);

This change missing "configuration" of usages in constructor of command and always validate on "default" usage with all arguments.

This change in behavior was intended?

That change was in my PR, definitely did not intend to break anything. Can you share a command definition that exhibits this problem so we can write a test?

kae commented

@aidanjryan
In my case it was

        [Oakton.Description("WebApi publish endpoint")]
	public class ListenCommand : BaseCommand<ListenInput> {
		public ListenCommand() {
			this.Usage("Listen to commands on default port").Arguments();
			this.Usage("Listen to commands").Arguments(i => i.Port);
		}
...

and model

	public class ListenInput : BaseInput {
		[Description("Listening on defined port")]
		public int Port { get; set; } = 12345;
		[Description("Get binaries"), FlagAlias("withBinaries", 'b')]
		public bool WithBinariesFlag { get; set; }
		[FlagAlias("restricted", 'r'), Description("Restrict to localhost")]
		public bool RestrictedFlag { get; set; }
	}

	public class BaseInput {
		[Description("Consul token"), FlagAlias("token", 't')]
		public string ConsulTokenFlag { get; set; }
		[Description("Consul HTTP endpoint address"), FlagAlias("http", 'h')]
		public string ConsulHttpFlag { get; set; }
		[Description("Timeout for waiting rpc response"), FlagAlias("timeout", 'i')]
		public int TimeoutFlag { get; set; } = 30;
		[Description("Environment name for settings discovery"), FlagAlias("env", 'e')]
		public string EnvironmentFlag { get; set; } = "local";
	}

with command line listen -e local

But I think it would break any case, with custom usage of 'optional' arguments

kae commented

@aidanjryan
If there are some questions about my example, I can provide more information.

@kae Sorry I'm just getting around to reproducing this. I see where building input from usages is failing when all arguments are optional. Could you explain what you meant here?

This change missing "configuration" of usages in constructor of command and always validate on "default" usage with all arguments.

@kae Nevermind! I see what you mean - when the usages are built in isolation without the command constructor self-configuring the usages, custom usages aren't present.

This is a catch-22 for the purpose of the "BeforeBuild" hook -- the idea was to allow a DI container to be configured based on the input, before commands are constructed, in case some DI config is required based on the input.

I think what's required is splitting BuildInput's two responsibilities of creating and validating the input. I'll work on a fix now.