Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for interactive mode. Fixes #12 #21

Merged
merged 8 commits into from
May 31, 2018
Merged

Added support for interactive mode. Fixes #12 #21

merged 8 commits into from
May 31, 2018

Conversation

RyanBruno
Copy link
Contributor

@RyanBruno RyanBruno commented May 27, 2018

This new update function simply removes the previous line and then optionaly calls another logger to replace it. An example use case would be:

signale.pending();
// Do something
signale.update(signale.complete, 'done');

The update function clears the previous line and calls the complete function with 'done' as its parameter.
Fixes #12

@klaudiosinani klaudiosinani self-requested a review May 27, 2018 14:15
@klaudiosinani klaudiosinani added the enhancement New feature or request label May 27, 2018
Copy link
Owner

@klaudiosinani klaudiosinani left a comment

Choose a reason for hiding this comment

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

Nice work! A few changes to be made and it is good to go : )

@jordrake
Copy link
Contributor

Would you mind updating this to handle the streams per type and multiple stream as in here?
#27

You'll need to expose the type's custom streams to do this.

@RyanBruno
Copy link
Contributor Author

RyanBruno commented May 30, 2018

It took me a few trys but I think I got it to work for both multiple and custom streams. @jordrake

@RyanBruno
Copy link
Contributor Author

@klauscfhq What other changes need to be made?

@klaudiosinani
Copy link
Owner

@RyanBruno A couple more tweaks in order to simplify the usage and creation of the interactive loggers, and it is good to be merged! : )

@RyanBruno
Copy link
Contributor Author

@klauscfhq Awesome! What tweaks specifically? The link in your previous comment was just https://github.com/klauscfhq/signale/pull/21/files

signale.js Outdated
@@ -257,6 +258,18 @@ class Signale {
return {label, span};
}
}

update(type, ...args) {
const streams = type.stream || this._formatStream(this._stream);
Copy link
Contributor

Choose a reason for hiding this comment

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

this._formatStream(type.stream || this._stream);

As type.stream can also be an array or a single stream.

@klaudiosinani
Copy link
Owner

klaudiosinani commented May 31, 2018

@RyanBruno My bad, here are the change notes:

signale.js@Line 3

The readline import can be removed, since we can call all the needed methods directly on the in-use writable stream:

const readline = require('readline'); // Can be omitted since it is a redundant requirement

signale.js@Line 262

We can ovoid calling the update() method manually, since that would be a very unintuitive way to use the interactive mode, and have it all done automatically by creating a new method, directly integrated inside of _log(), like so:

_write(stream, message) { // `_write()` can replace `update()` and simply use a boolean `this._interactive` variable to determine if the logger is in the interactive mode
  if (this._interactive) {
    // Note that `readline` is not needed, since all these methods can be directly called on the writable stream
    stream.moveCursor(0, -1); 
    stream.clearLine();
    stream.cursorTo(0);
  }
  stream.write(message + '\n');
}

_log(message, streams = this._stream) {
  this._formatStream(streams).forEach(stream => {
    // Use `_write()` inside of `_log()` to automatically achieve interactive messages.
    this._write(stream, message);
  });
}

signale.js@Line 20

Also, we should add the new boolean constructor attribute this._interactive, that will act as a user-option (e.g. const interactiveLogger = new Signale({interactive: true});) for creating interactive Signale instances. It can be included on line 20, just above the other attributes:

constructor(options = {}) {
   this._interactive = options.interactive || false; // We add `this._interactive` with a default state of `false`
   this._config = Object.assign(this.packageConfiguration, options.config);
   this._customTypes = Object.assign({}, options.types);
   this._scopeName = options.scope || '';
   this._timers = options.timers || new Map();
   this._types = Object.assign({}, types, this._customTypes);
   // ...
}

@klaudiosinani klaudiosinani changed the title Added update function Added support for interactive mode. Fixes #12 May 31, 2018
@RyanBruno
Copy link
Contributor Author

Ah. I understand what you are going for now. I swapped the update function for the interactive mode. Thanks for the help! Let me know if any more changes are needed.

@klaudiosinani
Copy link
Owner

Looks awesome! Thank you a lot for taking the time to contribute! : )

signale.js Outdated
@@ -251,6 +252,14 @@ class Signale {
return {label, span};
}
}

update(funct, ...args) {
readline.moveCursor(this._stream, 0, -1);
Copy link
Owner

Choose a reason for hiding this comment

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

It is possible to directly call these function on the in-use writable stream:

this._stream.moveCursor(0, -1);
this._stream.clearLine();
this._stream.cursorTo(0);

signale.js Outdated
@@ -251,6 +252,14 @@ class Signale {
return {label, span};
}
}

update(funct, ...args) {
Copy link
Owner

Choose a reason for hiding this comment

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

It is possible to avoid the process of having to manually call the update() function, in order for the last log message to be overridden, and have it done automatically by integrating it inside the _log() method.

_log(message) {
+  if (this._interactive) {
+    this._stream.moveCursor(0, -1);
+    this._stream.clearLine();
+    this._stream.cursorTo(0);
+  }
  this._stream.write(message + '\n');
}

The only extra addition should be a new boolean constructor attribute, e.g. this._interactive, that will act as an user option for creating an interactive Signale instance:

e.g. const interactiveLogger = new Signale({interactive: true});

The default state should be false

constructor(options = {}) {
+  this._interactive = options.interactive || false;
   this._config = Object.assign(this.packageConfiguration, options.config);
   this._customTypes = Object.assign({}, options.types);
   this._scopeName = options.scope || '';
   this._timers = options.timers || new Map();
   this._types = Object.assign({}, types, this._customTypes);
   this._stream = options.stream || process.stdout;
   // ...
}

signale.js Outdated
@@ -1,5 +1,6 @@
'use strict';
const path = require('path');
const readline = require('readline');
Copy link
Owner

Choose a reason for hiding this comment

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

This import can be removed since we are calling the needed methods directly on the in-use writable stream

Copy link
Owner

Choose a reason for hiding this comment

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

This still has to be removed since it is redundant

signale.js Outdated
@@ -257,6 +258,18 @@ class Signale {
return {label, span};
}
}

update(type, ...args) {
Copy link
Owner

Choose a reason for hiding this comment

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

For the same previously described reasons, there is no need to manually call the update() method, since that would be a very unintuitive way to use the interactive mode.

It can all be done automatically, by replacing update() with a new method, can be named _write() or _update() , which can be directly integrated inside of the already existing _log() , e.g:

_write(stream, message) {
  if (this._interactive) {
    // Note that `readline` is not needed, since all these methods can be directly called on the writable stream
    stream.moveCursor(0, -1); 
    stream.clearLine();
    stream.cursorTo(0);
  }
  stream.write(message + '\n');
}

_log(message, streams = this._stream) {
  this._formatStream(streams).forEach(stream => {
    // Use `_write()` inside of `_log()` to automatically achieve interactive messages.
    this._write(stream, message);
  });
}

Also, a new boolean constructor attribute is needed, e.g. this._interactive, so it can act as a user-option for creating interactive Signale instances:

e.g. const interactiveLogger = new Signale({interactive: true});

It can be added to the constructor like so:

constructor(options = {}) {
+  this._interactive = options.interactive || false;
   this._config = Object.assign(this.packageConfiguration, options.config);
   this._customTypes = Object.assign({}, options.types);
   this._scopeName = options.scope || '';
   this._timers = options.timers || new Map();
   this._types = Object.assign({}, types, this._customTypes);
   // ...
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🙋‍♂️ Interactive Logger
3 participants