Bring belochub/jstp-cli upstream #107

aqrln · 2017-03-28T16:55:51Z

I've created a branch to port belochub/jstp-cli to be a part of JSTP itself, as it was agreed upon in https://github.com/belochub/jstp-cli/issues/1.

Right now cli.js is located in the root of the project in order to mirror the original repo and make cherry-picking new commits effortless. It should later be separated into bin/cli.js and lib/cli/*.js.

/cc @belochub @lundibundi

aqrln · 2017-03-31T15:44:04Z

@lundibundi I've grabbed the latest patches.

aqrln · 2017-04-03T16:01:44Z

tools/cli.js

+  completer
+});
+
+const log = msg => {


@lundibundi can you please wrap msg into parens here?

@aqrln not a problem, should we add it to eslint (rule)?

This rule with as-needed and requireForBlockBody options enabled is exactly what we need. Would you like to open a PR for that?

aqrln · 2017-04-03T16:05:25Z

tools/cli.js

+  const processor = lineProcessor[type];
+  if (!processor) {
+    log(`Unknown command ${type}`);
+  } else {


I'd prefer either the if branch to return from the function and reduce the indentation level for the code inside the else branch or to flip the condition. I don't feel very strongly for one way or other, but I like the first one more.

The problem here is that using return is not the same as using else.

aqrln · 2017-04-03T16:06:41Z

tools/cli.js

+    log(`Unknown command ${type}`);
+  } else {
+    processor(leftover, (err, result) => {
+      if (err) return log(`${err.name} occurred: ${err.message}`);


I'd find it more readable to maybe use braces here.

aqrln · 2017-04-03T16:07:36Z

tools/cli.js

+};
+
+commandProcessor.call = (interfaceName, methodName, args, callback) => {
+  if (state.client === null) return callback(new Error('Not connected'));


if (!state.client) would be more readable here.

Not sure about whether the braces would help readability since the next line is quite long too.

@aqrln ok with !state.client, but the braces will not help that much

aqrln · 2017-04-03T16:08:22Z

tools/cli.js

+};
+
+commandProcessor.event = (interfaceName, eventName, args, callback) => {
+  if (state.client === null) return callback(new Error('Not connected'));


aqrln · 2017-04-03T16:09:35Z

tools/cli.js

+          log(`Received remote event: ${jstp.stringify(data)}`);
+        });
+        callback();
+      });


I'd prefer to put ); on a new line.

aqrln · 2017-04-03T16:09:57Z

tools/cli.js

+
+commandProcessor.connect = (host, port, appName, callback) => {
+  state.client = jstp.tcp.createClient({ host, port, secure: true });
+  state.client.connectAndHandshake(appName, null, null,


And maybe appName, null, null, too.

@aqrln not sure about this one, then wouldn't it be better to put each argument on separate line, but I see no reason for that here. @belochub what's your opinion?

It is okay either way, though I'd prefer to leave it as it is now.

aqrln · 2017-04-03T16:10:24Z

tools/cli.js

+};
+
+commandProcessor.disconnect = (callback) => {
+  if (state.client !== null) {


if (state.client) would be sufficient.

aqrln · 2017-04-03T16:11:53Z

tools/cli.js

+  const result = [];
+  let start = 0;
+
+  for (let i = 0; !limit || i < limit; i++) {


Is it intentional that zero limit is no limit?

aqrln · 2017-04-03T16:18:01Z

tools/cli.js

+      (err, connection) => {
+        if (err) return callback(err);
+        state.connection = connection;
+        // todo make event registering generic


The proper spelling of todo is TODO: :) That'll make it highlighted in code editors and picked up by bitHound to be shown on the dashboard.

aqrln · 2017-04-03T16:18:39Z

tools/cli.js

+  });
+};
+
+// map all remaining commands directly


Can you please start the comment with a capital letter?

eh, meme, okay

aqrln · 2017-04-03T22:34:25Z

@lundibundi for the record: commit your amendments based on the feedback from the review to this branch.

aqrln · 2017-04-07T12:05:45Z

@belochub and @lundibundi, thank you a lot, you've done a great job shaping it up. I'm going to land this so that new improvements can be done incrementally as separate PRs, but it needs reviewers to sign-off the commits. All your commits LGTM, but I need you to review my de10f00 and I'd also like to know whether you have reviewed each other's commits.

lundibundi · 2017-04-07T12:13:05Z

LGTM, it's good to be able to start it as usual shell command. Also yeah we have reviewed each other's commits so it's okay to land.

belochub · 2017-04-07T12:23:52Z

@aqrln, you forgot to add https://github.com/belochub/jstp-cli/commit/5ecc3ca46989f1976637be60888ab812c767fde1

aqrln · 2017-04-07T12:26:14Z

@belochub just did it ;) Thanks!

lundibundi

LGTM

belochub

LGTM, @aqrln, can you land it, please?

PR-URL: #107 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

They are shown in the line next to the one user has been printing in and new prompt is shown with the user input copied to it. PR-URL: #107 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

PR-URL: #107 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

Modify `_split` to push to array first substring of `str` which is not `splitter`, if limit is present and `leaveEmpty` set to false. PR-URL: #107 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

Move `cli.js` to `tools`, make it an executable script on its own and make NPM install an executable script named `jstp-cli` that points to `tools/cli.js`. PR-URL: #107 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

Prompt was not displayed if unknown command was entered. PR-URL: #107 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

Error message in 'disconnect' command was not transmitted correctly from commandProcessor by the lineProcessor to the caller. PR-URL: #107 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

This is a new and shiny first major release for `metarhia-jstp`. Changes include API refactoring and improvements, implementations of CLI, sessions, and application versions, native addon build optimizations, lots of bug fixes, test coverage increase, and other, less notable changes. This release also denotes the bump of the protocol version to v1.0. The only difference from the previous version of the protocol is that "old" heartbeat messages (`{}`) are now deprecated and `ping`/`pong` messages must be used for this purpose instead. Notable changes: * **src,build:** improve the native module subsystem *(Alexey Orlenko)* [#36](#36) **\[semver-minor\]** * **build:** compile in ISO C++11 mode *(Alexey Orlenko)* [#37](#37) **\[semver-minor\]** * **build:** improve error handling *(Alexey Orlenko)* [#40](#40) **\[semver-minor\]** * **lib:** refactor record-serialization.js *(Alexey Orlenko)* [#41](#41) **\[semver-minor\]** * **protocol:** change the format of handshake packets *(Alexey Orlenko)* [#54](#54) **\[semver-major\]** * **parser:** remove special case for '\0' literal *(Mykola Bilochub)* [#68](#68) **\[semver-major\]** * **client:** drop redundant callback argument *(Alexey Orlenko)* [#104](#104) **\[semver-major\]** * **client:** handle errors in connectAndInspect *(Alexey Orlenko)* [#105](#105) **\[semver-major\]** * **socket,ws:** use socket.destroy() properly *(Alexey Orlenko)* [#84](#84) **\[semver-major\]** * **cli:** add basic implementation *(Mykola Bilochub)* [#107](#107) **\[semver-minor\]** * **connection:** fix error handling in optional cbs *(Alexey Orlenko)* [#147](#147) **\[semver-major\]** * **lib:** change event signature *(Denys Otrishko)* [#187](#187) **\[semver-major\]** * **lib:** add address method to Server *(Denys Otrishko)* [#190](#190) **\[semver-minor\]** * **lib:** optimize connection events *(Denys Otrishko)* [#222](#222) **\[semver-major\]** * **lib:** refactor server and client API *(Denys Otrishko)* [#209](#209) **\[semver-major\]** * **lib,src:** rename term packet usages to message *(Denys Otrishko)* [#270](#270) **\[semver-major\]** * **lib:** emit events about connection messages *(Denys Otrishko)* [#252](#252) **\[semver-minor\]** * **connection:** make callback method private *(Alexey Orlenko)* [#306](#306) **\[semver-major\]** * **lib:** implement sessions *(Mykola Bilochub)* [#289](#289) **\[semver-major\]** * **connection:** use ping-pong instead of heartbeat *(Dmytro Nechai)* [#303](#303) **\[semver-major\]**

This is a new and shiny first major release for `metarhia-jstp`. Changes include API refactoring and improvements, implementations of CLI, sessions, and application versions, native addon build optimizations, lots of bug fixes, test coverage increase, and other, less notable changes. This release also denotes the bump of the protocol version to v1.0. The only difference from the previous version of the protocol is that "old" heartbeat messages (`{}`) are now deprecated and `ping`/`pong` messages must be used for this purpose instead. Notable changes: * **src,build:** improve the native module subsystem *(Alexey Orlenko)* [#36](#36) **\[semver-minor\]** * **build:** compile in ISO C++11 mode *(Alexey Orlenko)* [#37](#37) **\[semver-minor\]** * **build:** improve error handling *(Alexey Orlenko)* [#40](#40) **\[semver-minor\]** * **lib:** refactor record-serialization.js *(Alexey Orlenko)* [#41](#41) * **parser:** fix a possible memory leak *(Alexey Orlenko)* [#44](#44) **\[semver-minor\]** * **protocol:** change the format of handshake packets *(Alexey Orlenko)* [#54](#54) **\[semver-major\]** * **parser:** make parser single-pass *(Mykola Bilochub)* [#61](#61) * **parser:** remove special case for '\0' literal *(Mykola Bilochub)* [#68](#68) **\[semver-major\]** * **parser:** fix bug causing node to crash *(Mykola Bilochub)* [#75](#75) * **client:** drop redundant callback argument *(Alexey Orlenko)* [#104](#104) **\[semver-major\]** * **client:** handle errors in connectAndInspect *(Alexey Orlenko)* [#105](#105) **\[semver-major\]** * **socket,ws:** use socket.destroy() properly *(Alexey Orlenko)* [#84](#84) **\[semver-major\]** * **cli:** add basic implementation *(Mykola Bilochub)* [#107](#107) **\[semver-minor\]** * **connection:** fix error handling in optional cbs *(Alexey Orlenko)* [#147](#147) **\[semver-major\]** * **test:** add JSON5 specs test suite *(Alexey Orlenko)* [#158](#158) * **lib:** change event signature *(Denys Otrishko)* [#187](#187) **\[semver-major\]** * **lib:** add address method to Server *(Denys Otrishko)* [#190](#190) **\[semver-minor\]** * **parser:** implement NaN and Infinity parsing *(Mykola Bilochub)* [#201](#201) * **parser:** improve string parsing performance *(Mykola Bilochub)* [#220](#220) * **lib:** optimize connection events *(Denys Otrishko)* [#222](#222) **\[semver-major\]** * **lib:** refactor server and client API *(Denys Otrishko)* [#209](#209) **\[semver-major\]** * **lib,src:** rename term packet usages to message *(Denys Otrishko)* [#270](#270) **\[semver-major\]** * **lib:** emit events about connection messages *(Denys Otrishko)* [#252](#252) **\[semver-minor\]** * **lib:** implement API versioning *(Denys Otrishko)* [#231](#231) **\[semver-minor\]** * **lib:** allow to set event handlers in application *(Denys Otrishko)* [#286](#286) **\[semver-minor\]** * **lib:** allow to broadcast events from server *(Denys Otrishko)* [#287](#287) **\[semver-minor\]** * **connection:** make callback method private *(Alexey Orlenko)* [#306](#306) **\[semver-major\]** * **lib:** implement sessions *(Mykola Bilochub)* [#289](#289) **\[semver-major\]** * **connection:** use ping-pong instead of heartbeat *(Dmytro Nechai)* [#303](#303) **\[semver-major\]**

PR-URL: metarhia/jstp#107 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

* Fix code style * Fix error handling - show only error message without stacktrace * Make split work for separator of multiple characters * Fix map usage * Other bugfixes * General code refactoring PR-URL: metarhia/jstp#107 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

PR-URL: metarhia/jstp#107 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

PR-URL: metarhia/jstp#107 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

They are shown in the line next to the one user has been printing in and new prompt is shown with the user input copied to it. PR-URL: metarhia/jstp#107 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

PR-URL: metarhia/jstp#107 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

Modify `_split` to push to array first substring of `str` which is not `splitter`, if limit is present and `leaveEmpty` set to false. PR-URL: metarhia/jstp#107 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

Move `cli.js` to `tools`, make it an executable script on its own and make NPM install an executable script named `jstp-cli` that points to `tools/cli.js`. PR-URL: metarhia/jstp#107 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

Prompt was not displayed if unknown command was entered. PR-URL: metarhia/jstp#107 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

Error message in 'disconnect' command was not transmitted correctly from commandProcessor by the lineProcessor to the caller. PR-URL: metarhia/jstp#107 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>