dthree / vorpal

Node's framework for interactive CLIs
http://vorpal.js.org
MIT License
5.64k stars 280 forks source link

Print warning message if user registers an action with duplicate name #16

Closed melonmanchan closed 9 years ago

melonmanchan commented 9 years ago

Nice project you have there, thought I'd do my best to pitch in a bit.

This PR causes a nice obvious red warning message to be printed if an user attempts to register an action with the same name twice, in regards to issue #12 . Throwing an error would be another option, but I think printing a warning is a bit more user-friendly. Up to you, though :)

dthree commented 9 years ago

Thanks man :)

Could you change it up so that it's yellow? I generally make my warnings yellow as a standard. I also want to offer an instruction to them to delete the earlier command as the solution.

Something like this should work:

chalk.yellow("Warning: command named '"+ name + "' was registered more than once.\nIf you intend to override a command, you should explicitly remove the first command with command.remove().")

If you push that up, we should be good to go!

melonmanchan commented 9 years ago

Sure thing.

By the way, when is the cmd ._alias property initialized? I'm attempting duplicate alias detection too, but it seems like the property in question is not yet in the function where the registering happens?

melonmanchan commented 9 years ago

Ah, of course Vorpal's command-function simply returns the new command, which is then given an alias after that. So what would be the best place to check for duplicate aliases, seeming as the Command-objects have no knowledge of each other, as it should be. The show() function?

dthree commented 9 years ago

Nice.

On the alias thing, we should open a different PR / Issue, but:


function Command(name, parent) {
  // ...
  this._parent = parent;
  // ...
}

command.alias = function (alias) {
  this._aliases.push(alias);
  return this;
};

Check out the parent object passed into new Command(). I think that parent is the instance of Vorpal. So the check should be on the .alias method. If you are up to fixing this, I think the best bet is to extend this method, doing something like this.parent._commands.forEach... to check all other aliases and run a similar alert.

As there's no alias overwrite, honestly a duplicate alias should kill the app.

melonmanchan commented 9 years ago

I'll look into it!

dthree commented 9 years ago

:+1: