Completed nested command support. #245
Conversation
nowells
force-pushed
the
wireup-nested-commands
branch
2 times, most recently
from
c05a6c4
to
98d8154
Compare
|
Cool, thanks for the work! I can't test this right now as I'm away but if someone could test this out it'd be fantastic and we can get it merged. |
|
@thethomaseffect thanks! Let me know if there is anything I can do to help, or if you need me to fix/change the approach I have taken. |
nowells
force-pushed
the
wireup-nested-commands
branch
from
98d8154
to
85b98d6
Compare
| process.exit = function() { exceptionOccurred = true; throw new Error(); }; | ||
| console.error = function() {}; | ||
|
|
||
| try { |
There was a problem hiding this comment.
What's the purpose for these try/catch blocks? These are tests, we'd want all exceptions to bubble up.
There was a problem hiding this comment.
I was cribbing from https://github.com/visionmedia/commander.js/blob/master/test/test.options.commands.js#L103. Basically, we hijack process.exit(x) and have it throw an error (which help calls process.exit(0)) https://github.com/visionmedia/commander.js/pull/245/files#diff-48e1e0644a9c6aeb3fee25b9561b1692R60 We are OK with having the program exit (that is what help is supposed to do) but we want to assert that it was actually called (we add a on('help') above that sets the variable if help is called.
|
@SomeKittens have you run into any issues while testing/reviewing this code? No pressure, just want to see if there is anything I can do to help! |
Nested command support was almost completely available, however, a few things seemed to be missing to me. * `this` context on `action` event listeners was not bound properly. * We output `help` too early, the deepest referenced command should handle help.
nowells
force-pushed
the
wireup-nested-commands
branch
from
85b98d6
to
eda6a99
Compare
|
My first day with node-commander and I've already hit issues solved by this PR. Would be so good if someone could look at this but there seems to be many neglected PRs :( |
|
@ohjames We can't merge this - there's a bunch of conflicts... |
|
@ohjames bump |
|
Don't have the time to help out with this, found another library that supports it. |
|
@ohjames Which library? |
|
@jakiestfu it's been added to yargs with the |
|
@jakiestfu also just released https://github.com/apla/commop |
|
@apla thanks for the update, I'm your first star! Hoping you've finally made the holy grail of node.js cmd line argument parsers. |
|
Did this feature (nested commands) ever happen? |
|
@boutell nope I've been using yargs for the past few years instead. It's pretty good. |
|
I also switched to |
Nested command support was almost completely available, however, a few things
seemed to be missing to me.
thiscontext onactionevent listeners was not bound properly.helptoo early, the deepest referenced command should handle help.actionhandling logic to delegate to subcommands if they existed.This should address #226, and #1. Pretty much all of the support was already there, and it seemed useful to just connect the wires. I know that #63 referenced using the git subcommand model, which I love and use frequently, but there are several use cases (creating a command framework that loads external "plugin" module commands into the parent executable) that would be more easily expressed with this simple fix to connect the wires for sub-commands.