metarhia / jstp

Fast RPC for browser and Node.js based on TCP, WebSocket, and MDSF
https://metarhia.github.io/jstp
Other
142 stars 10 forks source link

cli: use new jstp features #366

Closed nechaido closed 6 years ago

nechaido commented 6 years ago

Changes:

belochub commented 6 years ago

We should also provide some kind of option (e.g. --no-reconnect with -r as an alias) to allow users to disable the default reconnector.

EDIT: Adding heartbeat interval option may be useful as well.

lundibundi commented 6 years ago

@belochub +1, but I think we can do that in follow-up PR. Also -r seems like it actually enables reconect, I'd suggest -R for negative aliases or not have this alias at all.

belochub commented 6 years ago

@lundibundi, yeah, -R is even better.

nechaido commented 6 years ago

+1 to @lundibundi.

belochub commented 6 years ago

@lundibundi, as an option, we can use cla-complete package, which just combines command-line-args and command-line-usage packages and handles parsing exceptions, but I don't think that it's really required here since such package can be created in minutes and we have only one CLI application to use it with, meaning we can easily write this ourselves and there is no need to abstract it now. As to the other points of your review, I think these couple of libraries are more flexible in terms of possible configuration and usage in some parts, and, as a result, using them will be more verbose (and requiring some manual checks), than using yargs.

nechaido commented 6 years ago

@lundibundi, as a matter of fact, it is a PR that must be landed ASAP. so that it is shipped with metarhia-jstp@2.

belochub commented 6 years ago

@lundibundi, as I said before, we can use cla-complete package instead of handling some of the things here, that way the code that you don't want to support will be "handled inside of the cli-args library", it's just going to be another library.

If the aforementioned package isn't good enough in your opinion, I can create another one, and just copy some of the code written by @nechaido in this PR into it, so that it will be handled inside the other package and not this one. That way you don't have to support the code that you don't want to support.

lundibundi commented 6 years ago

@belochub and there will be one more package to support, that you will have to update and test somehow. And most likely still not as good looking as yargs. To sum it up, if @nechaido and/or @aqrln agree that it's okay to have this instead of just using yargs I'll not object, as this will be better than having our own/your another package to support. (You can probably discuss it with them in person and just post the result/land it if it's okay)

belochub commented 6 years ago

@lundibundi, like I said, if you want the package that will handle the things you don't want to handle here, we can either use the cla-complete package or create another package with similar functionality.

there will be one more package to support

I don't understand, can you, please, explain what this other package to support will be?

you will have to update and test somehow

We don't have any tests for the cli at the moment.

belochub commented 6 years ago

Landed in https://github.com/metarhia/jstp/commit/ecf375da24481ee6fe995b1582eee9478f762932.