cli: use new jstp features #366

nechaido · 2018-08-09T16:14:28Z

Changes:

rely on a built-in reconnector;
add possibility to specify verboseness level.

lundibundi · 2018-08-09T17:09:40Z

lib/cli/cli.js

+          verboseLevel !== VERBOSE_LEVEL.ALL
+        ) {
+          return;
+        }


Could you reverse this and add a logging statement instead of return?

lundibundi · 2018-08-09T17:09:56Z

lib/cli/cli.js

+const serde = require('../serde');
+
+const VERBOSE_LEVEL = {
+  NO: 0,


Maybe Quiet?

lundibundi · 2018-08-09T17:10:14Z

lib/cli/cli.js

+          verboseLevel !== VERBOSE_LEVEL.ALL
+        ) {
+          return;
+        }


Same as above.

lundibundi · 2018-08-09T17:13:47Z

lib/cli/command-processor.js

+  }
+
+  connect(protocol, host, port, app, interfaces, callback) {
+    this.disconnect(() => {


Does this work? (I expect you to get Not connected error when you manually connect for the first time)

@lundibundi, error is ignored in the callback for the disconnect method here.

Although, I think it's better to make some kind of internal _disconnect method that doesn't check if the connection was established before and call it inside of both connect and disconnect methods. That way you can also avoid additional _connect method creation.

Yet one question stands: should we close a connection if the initial connection failed or should we let it reconnect?

I say we close it because this is probably because of an error in address, application name or method name.
@belochub @lundibundi what do you think?

belochub · 2018-08-09T17:55:18Z

bin/cli.js

+v - print every incoming and outgoing message exept heartbeat messages
+vv - print every incoming and outgoing message`,
+  })
+  .help()


This is a default value.

belochub · 2018-08-09T18:00:25Z

bin/cli.js

+    alias: 'v',
+    count: true,
+    describe: `Level of verboseness:
+v - print every incoming and outgoing message exept heartbeat messages


s/exept/except

belochub · 2018-08-09T18:15:16Z

lib/cli/command-processor.js

+  }
+
+  connect(protocol, host, port, app, interfaces, callback) {
+    this.disconnect(() => {


@lundibundi, error is ignored in the callback for the disconnect method here.

Although, I think it's better to make some kind of internal _disconnect method that doesn't check if the connection was established before and call it inside of both connect and disconnect methods. That way you can also avoid additional _connect method creation.

belochub · 2018-08-09T18:16:20Z

lib/cli/command-processor.js

-    this.cli.connection.emitRemoteEvent(interfaceName, eventName, args);
+    this.cli.connection.close();
+    this.cli.connection = null;
+    this.cli.connectInitiated = false;


You are not using this field anywhere else.

belochub · 2018-08-10T11:20:49Z

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 · 2018-08-10T11:42:54Z

@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 · 2018-08-10T11:44:13Z

@lundibundi, yeah, -R is even better.

belochub · 2018-08-10T12:49:34Z

lib/cli/cli.js

+
+      this.client = { logger };
+    } else {
+      this.client = null;


You can avoid this branch completely if you set this.client to an empty object before this if.

belochub

@nechaido, sorry, but I forgot that you also have to move yargs from devDependencies to regular dependencies.

belochub · 2018-08-10T16:34:34Z

bin/cli.js

+    alias: 'i',
+    type: 'number',
+    default: 0,
+    describe: 'Heartbeat interval in miliseconds',


s/miliseconds/milliseconds

belochub · 2018-08-15T13:11:18Z

bin/cli.js

+  args = commandLineArgs(options, { camelCase: true });
+} catch (error) {
+  console.error(error.message);
+  printHelp();


I don't think it should exit with code 0 when an error occurs during argument parsing.

belochub · 2018-08-15T13:13:17Z

bin/cli.js

+  },
+];
+
+const printHelp = () => {


Maybe printHelpAndExit will be a more informative name for this function?

belochub · 2018-08-15T13:14:28Z

bin/cli.js

+  printHelp();
+}
+
+if (args.help || args.heartbeatInterval === null) {


When heartbeatInterval is equal to null it should print some kind of an error message to tell the user that he is using it wrong, and it should exit with a non-zero code in that case.

belochub · 2018-08-15T13:21:32Z

lib/cli/cli.js

+      this.client.logger.on('outgoingMessage', (message) => {
+        if (
+          (!message.pong && !message.ping) ||
+          options.verbose === VERBOSE_LEVEL.ALL


I don't see where the verbose field is converted into a number in this implementation, thus it seems like you are checking for equality of a number and an array here, which will never evaluate to true.

belochub · 2018-08-15T13:34:27Z

bin/cli.js

+    header: 'JSTP CLI client',
+  },
+  {
+    header: 'Options',


What do you think about changing it to 'Available options'?

belochub · 2018-08-15T13:35:23Z

bin/cli.js

+  },
+  {
+    header: 'Options',
+    optionList: [


You can use the options array here if you move description and typeLabel fields there.

lundibundi

I don't think the fact that -help and --h are valid options (as this I assume the main reason for change) is worth changing the yargs to this library considering the verboseness and unsightliness (compared to yargs) of the new implementation.

Main points:

try/catch with args parsing (this should be handled internally)
no single-site to look at the options (options and their description is spread across options, usage and even arg checks later)
manual checking for invalid args (if (args.help || args.heartbeatInterval === null) {)
manual help checking/providing

I'd much rather have that slight deficiency with full and short option parsing than this lot of other things to deal with.

nechaido · 2018-08-15T14:29:05Z

+1 to @lundibundi.

Outdated

belochub · 2018-08-15T15:00:10Z

@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.

belochub · 2018-08-15T15:28:33Z

lib/cli/cli.js

+          (!message.pong && !message.ping) ||
+          options.verbose === VERBOSE_LEVEL.ALL
+        ) {
+          this.log(`Outgoing message:\n${serde.stringify(message)}`);


Must be "Incoming message" here.

belochub · 2018-08-15T15:29:24Z

bin/cli.js

+}
+
+if (args.heartbeatInterval === null) {
+  console.log('You must specify hearbeat interval');


s/hearbeat/heartbeat

Also, use console.error here, please.

belochub · 2018-08-15T16:33:58Z

bin/cli.js

+    name: 'heartbeat-interval',
+    alias: 'i',
+    type: Number,
+    default: 0,


The correct name of this property seems to be defaultValue.
Although I am not quite sure if we even need to use this property anywhere.

lundibundi · 2018-08-16T08:49:44Z

lib/cli/cli.js

 const utils = require('./utils');
+const serde = require('../serde');
+
+const VERBOSE_LEVEL = {


maybe VERBOSENESS_LEVEL or just VERBOSENESS? Though as you want.

lundibundi

I still think that we (or at least I) don't want to support code that should've been handled inside of the cli-args library.
Based on @nechaido comment I think that's at least 2 people who think that using yargs is better.
Therefore I'd want to discuss this in person or at least get @aqrln opinion on this here.
It's not a PR that needs to be landed asap so I think it can wait for a few more days.

nechaido · 2018-08-16T09:36:25Z

@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 · 2018-08-16T10:08:57Z

@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 · 2018-08-16T11:02:32Z

@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 · 2018-08-16T12:11:23Z

@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.

lundibundi

LGTM. Also, there is

jstp/lib/cli/command-processor.js

Line 137 in 6ec72a6

this.cli.connectInitiated = true;

do we still need to set it?

belochub · 2018-08-22T11:54:47Z

bin/cli.js

+    describe: 'Heartbeat interval in milliseconds',
+  }).argv;
+
+console.log(args);


Do we need it here?

belochub · 2018-08-22T14:29:13Z

lib/cli/command-processor.js

        callback(new Error(`Unknown protocol '${protocol}'`));
        return;
    }
    this.cli.connectInitiated = true;


You should probably remove this line as well.

belochub · 2018-08-22T14:35:57Z

bin/cli.js

+    count: true,
+    describe: 'Verbosely print incoming and outgoing messages',
+  })
+  .options('no-reconnect', {


It is options method here and option methods for 'verbose' and 'heartbeat-interval', is there a difference, or is this just a typo?

After taking a look at yargs documentation, it seems that the options method has a different contract, requiring to provide the object with options descriptions as a single argument, meaning that the way you use that method here is incorrect.

belochub · 2018-08-22T14:52:45Z

bin/cli.js

+const args = yargs
+  .option('verbose', {
+    alias: 'v',
+    count: true,


Why do you use count option here and not type like in other options definitions?

I need to use count so that when you pass -vv, verbose will be equal to 2. I do not use type here because this option does not have any value associated with but instead, we simply count the number of occurrences of this option.

I meant that you could've provided type: 'count' option instead of using count option, to make it consistent with other options definitions.

belochub · 2018-08-23T01:48:35Z

bin/cli.js

+    requiresArg: true,
+    type: 'number',
+    describe: 'Heartbeat interval in milliseconds',
+  }).argv;


In my opinion, we should also use strict() here.

Changes: * rely on a built-in reconnector; * add possibility to specify verboseness level.

* Rely on a built-in reconnector. * Add possibility to specify verboseness level. PR-URL: #366 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

belochub · 2018-08-23T12:20:49Z

Landed in ecf375d.

* Rely on a built-in reconnector. * Add possibility to specify verboseness level. PR-URL: #366 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

nechaido added the cli label Aug 9, 2018

nechaido requested review from belochub and lundibundi August 9, 2018 16:14

lundibundi reviewed Aug 9, 2018

View reviewed changes

belochub reviewed Aug 9, 2018

View reviewed changes

lundibundi previously approved these changes Aug 10, 2018

View reviewed changes

belochub reviewed Aug 10, 2018

View reviewed changes

nechaido force-pushed the fix-cli branch from 777d169 to cfd153e Compare August 10, 2018 14:14

belochub approved these changes Aug 10, 2018

View reviewed changes

belochub requested changes Aug 10, 2018

View reviewed changes

belochub reviewed Aug 15, 2018

View reviewed changes

lundibundi reviewed Aug 15, 2018

View reviewed changes

belochub reviewed Aug 15, 2018

View reviewed changes

belochub approved these changes Aug 15, 2018

View reviewed changes

nechaido requested a review from lundibundi August 15, 2018 19:15

lundibundi reviewed Aug 16, 2018

View reviewed changes

nechaido force-pushed the fix-cli branch from cd36c7e to ebb0a83 Compare August 21, 2018 15:51

nechaido force-pushed the fix-cli branch from ebb0a83 to 6ec72a6 Compare August 21, 2018 17:00

nechaido requested review from belochub and lundibundi August 22, 2018 12:41

lundibundi approved these changes Aug 22, 2018

View reviewed changes

belochub reviewed Aug 22, 2018

View reviewed changes

bin/cli.js Outdated

describe: 'Heartbeat interval in milliseconds',

}).argv;

console.log(args);

Copy link

Member

belochub Aug 22, 2018

Choose a reason for hiding this comment

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

Do we need it here?

belochub reviewed Aug 22, 2018

View reviewed changes

nechaido force-pushed the fix-cli branch from d069e8a to fa15ec3 Compare August 22, 2018 18:08

belochub approved these changes Aug 23, 2018

View reviewed changes

belochub reviewed Aug 23, 2018

View reviewed changes

cli: use new jstp features

5085e80

Changes: * rely on a built-in reconnector; * add possibility to specify verboseness level.

nechaido force-pushed the fix-cli branch from fa15ec3 to 5085e80 Compare August 23, 2018 08:16

belochub approved these changes Aug 23, 2018

View reviewed changes

belochub closed this Aug 23, 2018

belochub deleted the fix-cli branch August 23, 2018 12:20

belochub mentioned this pull request Aug 30, 2018

v2.0.0 proposal #375

Merged

cli: use new jstp features #366

cli: use new jstp features #366

Uh oh!

Conversation

nechaido commented Aug 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

belochub commented Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lundibundi commented Aug 10, 2018

Uh oh!

belochub commented Aug 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

belochub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lundibundi left a comment

Choose a reason for hiding this comment

Uh oh!

nechaido commented Aug 15, 2018

Uh oh!

belochub commented Aug 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lundibundi left a comment

Choose a reason for hiding this comment

Uh oh!

nechaido commented Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

belochub commented Aug 16, 2018

belochub commented Aug 10, 2018 •

edited

Loading

nechaido commented Aug 16, 2018 •

edited

Loading