Skip to content

Pass unknown also as a second argument to the command:* event#654

Closed
jseminck wants to merge 1 commit intotj:masterfrom
jseminck:unknown-for-all-commands
Closed

Pass unknown also as a second argument to the command:* event#654
jseminck wants to merge 1 commit intotj:masterfrom
jseminck:unknown-for-all-commands

Conversation

@jseminck
Copy link

Thanks a lot for the amazing library.

I was browsing through the code to find a way to get access to the unknown options array that is currently only exposed through the command:${commandName} event. But this event also calls .shift() on the arguments array, which is not something that we'd want.

Therefore I was wondering, if there is a specific reason that the unknown array is not passed as a second argument to the command:* event?

I added it there, but had to fix one test as this was passing unknown options and without specifying that unknown options are allowed, the test would crash now.

I also added a test case to actually test that the unknown array is being passed as the second argument in both command:[*|commandName] events.

@GreenGremlin
Copy link

I was just wondering the same thing. It would be nice to be able to access unknown options from the action as well. This could be done by pushing unknown to args before calling fn.apply.

Something like this

Command.prototype.action = function(fn) {
  var self = this;
  var listener = function(args, unknown) {
    ...

    if (self._args.length) {
      args[self._args.length] = self;
    } else {
      args.push(self);
    }
    args.push(unknown); // Adding this line would give the action callback access to the unknown options array as well.

    fn.apply(self, args);
  };
  ...
};

Or maybe even expose the unknown options on the commander instance so a user could access them via program.unknownOptions.

@shadowspawn
Copy link
Collaborator

@GreenGremlin I think a method on the commander object would be tidier than pushing arguments into callback.

Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

Are you still interested in doing more work on this @jseminck ?

It seems reasonable to make the two calls more consistent. (The named command emit does get passed the commander object as an argument well, but that is a separate issue.) There is the risk of breaking existing code so the change might have to was it for a major version bump.

I have some changes I would like made. Checking whether you are still engaged before I go further.

No problem if you are no longer interested, I'll find somewhere to reference this work for future use by someone else.

Thank you for your contributions.

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