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

Prevent aliases already in use and subcommands with such an alias or the name already in use from being added #1924

Closed

Conversation

aweebit
Copy link
Contributor

@aweebit aweebit commented Aug 1, 2023

Pull Request

Problem

Adding aliases that are already in use and subcommands with such an alias or the name that is already in use is allowed.

Demo

program.command('sub').action(() => console.log('first'));
program.command('sub').action(() => console.log('second'));
program.parse(['sub'], { from: 'user' }); // output will always be first
program.command('sub').action(() => console.log('first'));
const explicit = new Command('explicit')
  .alias('sub')
  .action(() => console.log('second'));
program.addCommand(explicit);
program.parse(['sub'], { from: 'user' }); // output will always be first
program.command('sub');
const explicit = new Command('explicit');
program.addCommand(explicit);
explicit.alias('sub'); // does not throw

Original issue

ChangeLog

Changed

  • Breaking: throw an error when trying to add a subcommand with a name or an alias that is already in use
  • Breaking: throw an error when trying to add an alias that is already in use to a subcommand

Peer PRs

…solving similar problems

@shadowspawn
Copy link
Collaborator

I had done some thinking when issue first added, and aliases are a complication. With current PR code aliases on the first command get checked when the second command is added. But I think not vice versa.

Some possibilities:

  • don't worry, covered much more than before!
  • do the whole conflict command check from start of parse rather than as command added (this is reasonable, just slightly annoying to mix logic). Still highly likely to be detected by author rather than found by end-user, plugins excepted.
  • check at runtime and throw if ambiguous name used (bad, because depends on parameters used so may be missed by author. But mentioned it because I considered it!)

@shadowspawn shadowspawn added the semver: major Releasing requires a major version bump, not backwards compatible label Aug 1, 2023
@shadowspawn shadowspawn changed the base branch from develop to release/12.x August 1, 2023 08:52
@aweebit aweebit changed the title Prevent subcommands with name already in use from being added Prevent aliases already in use and subcommands with such an alias or the name already in use from being added Aug 1, 2023
@aweebit
Copy link
Contributor Author

aweebit commented Aug 1, 2023

I had done some thinking when issue first added, and aliases are a complication. With current PR code aliases on the first command get checked when the second command is added. But I think not vice versa.

Some possibilities:

  • don't worry, covered much more than before!
  • do the whole conflict command check from start of parse rather than as command added (this is reasonable, just slightly annoying to mix logic). Still highly likely to be detected by author rather than found by end-user, plugins excepted.
  • check at runtime and throw if ambiguous name used (bad, because depends on parameters used so may be missed by author. But mentioned it because I considered it!)

I have come up with a better solution, check bd12c90 and the updated PR description.

@aweebit
Copy link
Contributor Author

aweebit commented Aug 1, 2023

I have come up with a better solution, check bd12c90 and the updated PR description.

My suggestion from the first comment in #1917 enabling sharing and stand-alone use of subcommands won't work if this change is accepted, though. If we decide to implement the suggestion, the checks will have to be done at the beginning of parse calls as you suggested, which I honestly don't like because parse calls are only supposed to throw exceptions from user code (argParsers, hooks, actions), so might be a good idea to just dump that suggestion of mine.

Update: There will be no problem if all parent commands are stored, see #1938 for an implementation. When that PR is merged, the check in alias() will simply have to iterate over all of them.

@shadowspawn
Copy link
Collaborator

shadowspawn commented Aug 21, 2023

The way commands with alias get displayed in the help is like one|1, so a compact way to describe command name and an alias.

I tried a rework to use that form in error messages:

  _checkForConflictingCommandNames(name, aliases = []) {
    let newKey;
    let oldKey;
    const makeKey = (nameOrAlias, matchingCommand) => {
      if (nameOrAlias === matchingCommand.name()) return nameOrAlias;
      return `${matchingCommand.name()}|${nameOrAlias}`;
    };
    let matchingCommand = this._findCommand(name);
    if (matchingCommand) {
      newKey = name;
      oldKey = makeKey(name, matchingCommand);
    } else {
      const alias = aliases.find((alias) => {
        matchingCommand = this._findCommand(alias);
        return matchingCommand;
      });
      if (matchingCommand) {
        newKey = `${name}|${alias}`;
        oldKey = makeKey(alias, matchingCommand);
      }
    }
    if (matchingCommand) {
      throw new Error(`Cannot add command '${newKey}'${this._name && ` to '${this._name}'`} since command '${oldKey}' has already been added`);
    }
  }

Example error when use addCommand() and alias clashes with alias :

Error: Cannot add command 'three|1' since command 'one|1' has already been added

@shadowspawn
Copy link
Collaborator

shadowspawn commented Nov 4, 2023

Closing in favour of #2059 (added co-author on that PR).

@shadowspawn shadowspawn closed this Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: major Releasing requires a major version bump, not backwards compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants