search

dthree / vorpal

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

Do not attach ui after parse #200

Closed ccorcos closed 7 years ago

ccorcos commented 7 years ago

There are a few reasons for this PR.

  1. I think its a good idea to be able to use Vorpal simply as a CLI tool without necessarily having to use the interactive shell when using .parse

  2. I was having some issues where I was starting an express server with a Vorpal command and I would listen to SIGINT and SIGTERM to gracefully shutdown the server, but then I would get this bizarre Error: read EIO close error on the next key I typed after exiting with ctrl-c. The only way to solve it was with process.on('SIGINT', () => process.exit(2)). I don't get this error anymore.

Reference Issue: https://github.com/dthree/vorpal/issues/199

I meant to open up a second PR, but it ended up here as well so why not: https://github.com/dthree/vorpal/issues/198

It's just adding a simple prototype method on command so you can compose program options. for example.

ccorcos commented 7 years ago

Also, @dthree after reading your notice, I think I could offer some help. I'm building a project that makes heavy use of Vorpal :) https://github.com/ccorcos/doug/

ccorcos commented 7 years ago

@scotthovestadt mind taking a look at this?

LongLiveCHIEF commented 7 years ago

I've been having similar issues with vorpal over the last year. We need to make sure that this patch doesn't break any other functionality, and I know we're light on tests.

One of the other issues I've had to work around is the usage of this within action, and I have a feeling this patch will alter this behavior.

We def need some tests for the functionality this patch brings.

I'm not sure how using void 0 is changing any functionality. here since it's being assigned to a variable. Can you speak to what that change fixes?

dthree commented 7 years ago

I think this is the most important patch right now. About 4 months ago, some PR broke the parse functionality, so I'm all for this fix.

ccorcos commented 7 years ago

@LongLiveCHIEF I didnt make any changes to dist other than running gulp build. My changes are just in the two lib files that are super simple

ccorcos commented 7 years ago

Let me know if there's anything else I can do or when this is published :)

LongLiveCHIEF commented 7 years ago

My bad, I'm so used to /dist being ignored by git that I didn't put 2 and 2 together. I just joined the project, so wanted to make sure the important patches got reviewed first and foremost.

LongLiveCHIEF commented 7 years ago

I think this is the most important patch right now. About 4 months ago, some PR broke the parse functionality, so I'm all for this fix.

I was also going over some of the details of #169 and can see there has been a lot of discussion around parse

@dthree all said... what do you think is needed in order to accept this PR?

dthree commented 7 years ago

This looks fine as is - it's actually a revert really.

johnthepink commented 7 years ago

@dthree would love to get this in a release <3

ccorcos commented 7 years ago

@dthree can you please publish this?