apple / swift-argument-parser

Straightforward, type-safe argument parsing for Swift
Apache License 2.0
3.3k stars 311 forks source link

Add aliases support for sub commands #627

Closed dcantah closed 3 months ago

dcantah commented 4 months ago

This adds support for aliases for subcommands via a new parameter to CommandConfiguration's constructors. The aliases are passed as an array of strings, where the default is just an empty array that signifies there are no aliases. The aliases are supported regardless of if a different commandName is chosen or not. This also updates how subcommands show up in the help text; any aliases are now displayed to the right of the original command.

In addition to the functionality itself, this change:

  1. Updates some of the EndToEnd parsing tests to make sure they function while using aliases.
  2. Sprinkles mentions where I saw fit in the documentation.
  3. Updates the Math example to have aliases for math stats average (math stats avg), and math multiply (math mul).

math's help text now looks like the below:

~ math --help
OVERVIEW: A utility for performing maths.

USAGE: math <subcommand>

OPTIONS:
  --version               Show the version.
  -h, --help              Show help information.

SUBCOMMANDS:
  add (default)           Print the sum of the values.
  multiply, mul           Print the product of the values.
  stats                   Calculate descriptive statistics.

  See 'math help <subcommand>' for detailed help.

~ math stats --help
OVERVIEW: Calculate descriptive statistics.

USAGE: math stats <subcommand>

OPTIONS:
  --version               Show the version.
  -h, --help              Show help information.

SUBCOMMANDS:
  average, avg            Print the average of the values.
  stdev                   Print the standard deviation of the values.
  quantiles               Print the quantiles of the values (TBD).

  See 'math help stats <subcommand>' for detailed help.

and use of the aliases:

~ math mul 10 10
100

~ math stats avg 10 20
15.0

This change does NOT add any updates to the shell completion logic for this feature.

Fixes #248

Checklist

dcantah commented 4 months ago

@rauhul With the current implementation I think it'd basically be a no-op (but also just look dumb in the help text). The old tree parsing would stop if the current subcommand being checked against matched the value. The new logic just adds a case to check if the current value matches any of the aliases for that subcommand also. So fairly sure all that would happen is we'd take the first case as we would prior. Would you prefer to throw in some way (I'm leaning towards yes myself)?

rauhul commented 4 months ago

@rauhul With the current implementation I think it'd basically be a no-op (but also just look dumb in the help text). The old tree parsing would stop if the current subcommand being checked against matched the value. The new logic just adds a case to check if the current value matches any of the aliases for that subcommand also. So fairly sure all that would happen is we'd take the first case as we would prior. Would you prefer to throw in some way (I'm leaning towards yes myself)?

I would personal prefer an assertion failure or precondition failure, not sure which right now.

dcantah commented 4 months ago

Ack, I'll see if we have any similar validations and how they're handled for now

natecook1000 commented 4 months ago

@dcantah Thanks for working on this! There's a set of validations that run in debug mode – this should have its own so that we don't end up with ambiguous commands. It should be okay to have duplicate names as long as they're in different parts of a command hierarchy, or we could disallow that as well to be clearer.

dcantah commented 4 months ago

A pretty decent spot to plop these checks are in the convenience constructor for Tree, this already has a InitializationError that handles if a sub command has a recursive entry of itself. Now, that seems much more fatal than our case, and indeed y'all use fatalError when catching the error, but this seems a decent spot to introduce a new error (whether it be fatal, precondition, assert etc.):

do {
    self.commandTree = try Tree(root: rootCommand)
} catch Tree<ParsableCommand.Type>.InitializationError.recursiveSubcommand(let command) {
    fatalError("The ParsableCommand \"\(command)\" can't have itself as its own subcommand.")
} catch Tree<ParsableCommand.Type>.InitializationError.aliasMatchingCommand(let command) {
    fatalError("The ParsableCommand \"\(command)\" can't have an alias with the same name as the command itself.")
} catch {
    fatalError("Unexpected error: \(error).")
}

The other case I'm thinking is there's really no reason to have aliases for the root command, so possibly another error here for that case. I can fix up the documentation for the above case, and this one if we agree on not allowing root commands to have aliases.

dcantah commented 4 months ago

Updated with a couple new tests to trial out not allowing an alias to match the commands name, as well as documented this behavior. I went with the approach we have right now for Tree initialization failures, which is this is treated as a fatalError. If we want to lessen this let me know

natecook1000 commented 4 months ago

I think the situation we're looking to prevent is one subcommand's alias colliding with a sibling subcommand's name or alias (for example, two subcommands named subtract and substitute could both define a sub alias). The validator should do something like traversing the command hierarchy tree, collecting all the names/aliases of subcommands, and making sure there are no duplicates.

That said, I don't think we perform this validation right now for subcommand names! So I don't think this validation is a strict requirement here, and could be a usability improvement in a follow-up PR.

dcantah commented 4 months ago

Great point @natecook1000. I opened up a change here to add in general ambiguous subcommand name detection. This can be expanded upon for aliases fairly easily I think https://github.com/apple/swift-argument-parser/pull/629

dcantah commented 4 months ago

@swift-ci please test

dcantah commented 4 months ago

@swift-ci please test