@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 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.

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

@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.

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.

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

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.

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 #629

@swift-ci please test

@swift-ci please test