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

Introduce CliAction, make it possible for every Symbol to define one #2095

Merged
merged 8 commits into from
Mar 16, 2023

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Mar 15, 2023

Overall it was way easier than I expected.

This PR:

  1. Replaces ICommandHandler with CliAction
  2. Renames Command.SetHandler with Command.SetAction.
  3. Changes the exit code approach: every successful program should return 0, so to avoid verbosity command actions provided by Command.SetAction do not need to return an int. In case somebody wants to reuturn an int, they need to implement their own CliAction type (pay for play)
  4. Extends Options and Directives with Action property, but not SetAction methods (no need to, as it should be rare to have active options and directives that don't need custom types)
  5. It does not expose the new action types, I want to do it in a separate PR, where I am going to write tests for them and move the configuration to these types.

Contributes to #2071

@adamsitnik adamsitnik requested review from jonsequitur and Keboo March 15, 2023 10:49
@@ -346,5 +346,38 @@ public void Option_of_enum_can_limit_enum_members_as_valid_values()
.Should()
.BeEquivalentTo(new[] { $"Argument 'Fuschia' not recognized. Must be one of:\n\t'Red'\n\t'Green'" });
}

[Fact]
public void Every_option_can_provide_a_handler_and_it_takes_precedence_over_command_handler()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if two different options having handlers are both present?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have two possibilities:

  • stop on the first active option
  • let the last one to be the effective (this is what we already do for commands)

@jonsequitur choosing the last one is the most intuitive way for me. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also make it a parse error. Of course certain options (like -h) ignore that and the action still runs.

@jonsequitur
Copy link
Contributor

I am not sure whether it should be allowed for Arguments, currently the argument handlers are ignored.

I wasn't expecting this to be available on Argument and I'm not sure how to reason about it. I don't believe I've seen a CLI where I would model something this way.

expose Action only for Command, Directive and Option
expose SetAction only for Command, make it an actual Action (no exit code returning)
@adamsitnik
Copy link
Member Author

I wasn't expecting this to be available on Argument and I'm not sure how to reason about it. I don't believe I've seen a CLI where I would model something this way.

That is very true. I've applied following changes:

  • Command, Option and Directive expose CliAction Action property
  • only Command exposes SetAction methods (for options and directives it's too rare to pollute the public API surface)

@adamsitnik
Copy link
Member Author

We could also make it a parse error. Of course certain options (like -h) ignore that and the action still runs.

Sure, let's discuss it offline with @KathleenDollard. I am going to merge this PR right now to avoid conflict with my next PR (removal of middleware!)

@adamsitnik adamsitnik merged commit 9e4d60b into dotnet:main Mar 16, 2023
@adamsitnik adamsitnik deleted the cliAction branch March 16, 2023 13:41
@tmds
Copy link
Member

tmds commented Mar 19, 2023

@adamsitnik in #2046 you said: all handlers should return an int.

And here the design is:

Command.SetAction do not need to return an int. In case somebody wants to reuturn an int, they need to implement their own CliAction type (pay for play)

Failures are often expected in command line applications.

Without the capability to return an int, command handler set using SetAction are forced to throw for failure.

This lead to a bad user experience.
And, throwing exceptions for expected failures is a performance anti-pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants