Skip to content
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

Consider renaming the API ThrowOnUnexpectedArgument #88

Closed
parekhkb opened this issue Apr 20, 2018 · 13 comments · Fixed by #340 or #341
Closed

Consider renaming the API ThrowOnUnexpectedArgument #88

parekhkb opened this issue Apr 20, 2018 · 13 comments · Fixed by #340 or #341
Assignees
Milestone

Comments

@parekhkb
Copy link

I am trying to make my app not throw an exception if the user enters an invalid command/option/argument.

I set ThrowOnUnexpectedArgument to false in the constructor of the CommandLineApplication. I also set a ValidationErrorHandler function.

However, an exception is still thrown.

image

@parekhkb parekhkb changed the title App is still throwing CommandParsingException even if ThrowOnUnexpectedArgument = False is not working App is still throwing CommandParsingException even if ThrowOnUnexpectedArgument = False Apr 20, 2018
@natemcmaster
Copy link
Owner

Can you provide repro steps? I have a guess, but it's hard to tell without knowing what is being passed in as args[] and what CommandBase.ConfigurationApplication is doing in your code.

@natemcmaster natemcmaster added the more-info-needed Please provide more information. label Apr 20, 2018
@parekhkb
Copy link
Author

parekhkb commented Apr 20, 2018

Im calling dotnet MyApp.dll foo or dotnet MyApp.dll --foo both repro the issue.

Below are some relevant code snippets:

        private void ConfigureCommand(CommandLineApplication command, IServiceCollection services)
        {
            _services = services;

            command.Description = Description;
            command.HelpOption("-?|-h|--help");

            ConfigureCommandInternal(command);

            command.OnExecute(() => OnExecute(command));
        }
        protected override void ConfigureCommandInternal(CommandLineApplication command)
        {
            // Register sub-commands
            RegisterSubCommand<SubCommand1>(command);
            RegisterSubCommand<SubCommand2>(command);
            RegisterSubCommand<SubCommand3>(command);
            RegisterSubCommand<SubCommand4>(command);
        }
        public CommandLineApplication RegisterSubCommand<TCommand>(TCommand com, CommandLineApplication root) where TCommand : CommandBase
        {
            return root.Command(com.Name.ToLower(), c => com.ConfigureCommand(c, _services));
        }

        public CommandLineApplication RegisterSubCommand<TCommand>(CommandLineApplication root) where TCommand : CommandBase, new()
        {
            return RegisterSubCommand(new TCommand(), root);
        }

@natemcmaster
Copy link
Owner

natemcmaster commented Apr 21, 2018

Try changing this:

        public CommandLineApplication RegisterSubCommand<TCommand>(TCommand com, CommandLineApplication root) where TCommand : CommandBase
        {
-            return root.Command(com.Name.ToLower(), c => com.ConfigureCommand(c, _services));
+            return root.Command(com.Name.ToLower(), 
+              c => com.ConfigureCommand(c, _services),
+              throwOnUnexpectedArg: false);
        }

@natemcmaster

This comment has been minimized.

@parekhkb
Copy link
Author

Thanks, @natemcmaster, that was indeed the issue. However I think there might be another bug: If I use throwOnUnexpectedArg: false. Then My OnValidationError Handler never get's called, and the app returns 0 rather than non-zero. I ended up just catching the ComandParsingException myself in order to write a useful message, eat the error, and return -1.

@natemcmaster
Copy link
Owner

What kind of validators have you configured? The app should return 1 by default if there are validation errors. ValidationErrorHandler is not invoked if there were not validation errors.

By the way, each subcommand is a new instance of CommandLineApplication, each with its own ValidationErrorHandler. Are you invoking a subcommand, but expecting the validation error handler on the root CommandLineApplication object to be called?

@parekhkb
Copy link
Author

I see. Thanks for your help. I guess the issue I have with this feature is that it doesn't exit execution onValidationError.

@natemcmaster
Copy link
Owner

I think there is probably a bug here somewhere, I'm just not sure what it is. So far, I've identified at least two things that are not clear about the behavior of this API

  • throwOnUnexpectedArgs has to applied to all subcommands explicitly. A better default may be to honor the value set on the root command instead of defaulting to false
  • ValidationErrorHandler is only invoked on the command with the validation error, so you end up re-defining a customer error handler on all subcommand objects

Does this describe your issues so far? I'm not sure what you meant by the most recent comment about "it doesn't exit execution onValidationError."

@parekhkb
Copy link
Author

parekhkb commented Apr 25, 2018

Yes those two describe issues I've seen so far.

Here's what I mean by it doesn't exit execution onValidationError.

If I set throwOnUnexpectedArgs to false for the root and all sub-commands, and I specify an OnValidation() handler for each command, and my handler returns 3 (non-zero), then I would expect the app to exit after the onValidation handler is executed and the args are found to be invalid. Instead it proceeds to execute my app. For example. If I call dotnet MyApp.dll ValidCommand --valid-option --invalid-option, i would expect the existence of --invalid-option to prevent the app from doing what it would do if I just ran dotnet MyApp.dll ValidCommand --valid-option

To give you some context, this issue started when I tried to stop bad arguments from printing stack traces. I want bad arguments to fail the execution, but not print out exception stack traces.

@natemcmaster
Copy link
Owner

Oh, I see. Ok, so maybe this explanation will help.

What ThrowOnUnexpectedArgs:false means to the parser is "parse until you hit an unrecognized argument or option. Then, but everything else into the CommandLineApplication.RemainingArguments property".

For example:

Input ValidOption.HasValue() RemainingArguments (string[])
ValidCommand --valid-option --invalid-option true { "--invalid-option" }
ValidCommand --invalid-option --valid-option false { "--invalid-option", "--valid-option" }
ValidCommand banana --invalid-option --valid-option false { "banana", "--invalid-option", "--valid-option" }

This behavior was added before this library every had support for validation on arguments. I realize now why this behavior isn't obvious. It's probably time to reconsider what the "throwOnUnexpectedArgs" value means.

@natemcmaster natemcmaster reopened this Apr 26, 2018
@natemcmaster natemcmaster added needs-design and removed more-info-needed Please provide more information. labels May 31, 2018
@natemcmaster natemcmaster changed the title App is still throwing CommandParsingException even if ThrowOnUnexpectedArgument = False Consider renaming the API ThrowOnUnexpectedArgument May 31, 2018
@natemcmaster natemcmaster added this to the 3.0.0 milestone Jul 5, 2018
@natemcmaster natemcmaster added the help wanted We would be willing to take a well-written PR to help fix this. label Jan 6, 2019
@natemcmaster natemcmaster removed this from the 3.0.0 milestone Jan 6, 2019
@stale
Copy link

stale bot commented Apr 6, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please comment if you believe this should remain open, otherwise it will be closed in 7 days.

@stale stale bot added the closed-wontfix This issue is closed because there are no plans to fix this. label Apr 6, 2019
@stale stale bot closed this as completed Apr 13, 2019
@natemcmaster natemcmaster added closed-stale This issue is closed because it went stale and there was no action planned. It can be reopened. and removed closed-wontfix This issue is closed because there are no plans to fix this. labels May 17, 2019
@natemcmaster natemcmaster removed closed-stale This issue is closed because it went stale and there was no action planned. It can be reopened. needs-design labels Jul 15, 2019
@natemcmaster
Copy link
Owner

Stale bot closed this, but I'd like to revisit this in 3.0. Reopening

@natemcmaster
Copy link
Owner

Finally got around to this. I've renaming the API. See #340 and #339. My plan is to obsolete the API in 2.6 and delete in 3.0. I'll be merging the changes soon to close this out, but it will probably take a few weeks to make any releases to nuget.org.

@natemcmaster natemcmaster removed the help wanted We would be willing to take a well-written PR to help fix this. label Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment