Added support for string interpolation. Fixes #20 #23

rjoydip-zz · 2018-05-27T08:56:35Z

This PR provides a mechanism for support printf-like formatting. Strings have been formatted with util.format().

Fixes #20 , #24

klaudiosinani

Nice work! Only a few changes are needed to get this merged : )

nick-keller · 2018-05-29T14:44:55Z

Awesome PR!

markusberg-spt · 2018-06-06T13:25:40Z

Awesome feature! Is this going to be merged? @rjoydip @klauscfhq

klaudiosinani · 2018-06-06T13:34:42Z

@novddd A couple of tweaks and it will be good to go 🚀

klaudiosinani · 2018-05-27T15:07:49Z

signale.js

        additional = Object.assign({}, {suffix, prefix});
      }
    } else {
-      msg = args.join(' ');
+      msg = util.format(args[0], ...args.slice(1, args.length));


To simplify it even more, the args argument can be passed directly to the util.format() method using the rest parameter syntax:

msg = util.format(...args);

klaudiosinani · 2018-05-27T15:09:05Z

signale.js

-        const [{prefix, message, suffix}] = args;
-        msg = message;
+        const [{prefix, suffix}] = args;
+        msg = util.format(util.inspect(...args));


The same can be done here:

msg = util.format(...message);

klaudiosinani · 2018-05-27T15:10:43Z

signale.js

      } else {
-        const [{prefix, message, suffix}] = args;


Here message is needed for destructuring args

I have removed message because now it became death code.
Here is the senerior which i am testing and getting { prefix: undefined, message: undefined, suffix: undefined }

signale.success({a: 2}); signale.success([1, 2]);

Can you give some use cases so tat i can check that.

klaudiosinani · 2018-05-27T15:13:52Z

signale.js

@@ -131,14 +132,14 @@ class Signale {

    if (args.length === 1 && typeof (args[0]) === 'object' && args[0] !== null) {
      if (args[0] instanceof Error) {
-        [msg] = args;
+        [msg] = args; // See this github issue comment (https://github.com/nodejs/node/issues/9406#issuecomment-257724625)


Error instances can receive only a single string as argument on initialization, thus string interpolation is expected to not be applicable here, which make the comment redundant

klaudiosinani · 2018-05-27T15:14:05Z

.gitignore

@@ -2,6 +2,9 @@
 node_modules
 yarn.lock

+# folder


This one should be removed since it is not part of the default developing environment

That's my fault. I am removing this.

rjoydip-zz · 2018-06-06T19:53:58Z

Soon I will do requested changes.

…sue-20

klaudiosinani · 2018-06-09T14:44:26Z

signale.js

@@ -161,12 +162,12 @@ class Signale {
      if (args[0] instanceof Error) {
        [msg] = args;
      } else {
-        const [{prefix, message, suffix}] = args;


Looks nice! The only thing left is to bring back message for destructuring args :

const [{prefix, message, suffix}] = args;

and directly apply interpolation on it:

msg = util.format(...message);

Thank you a lot for taking the time to help out : )

Yes, that I have to do. I am getting {prefix, message, suffix} as undefined all the times

Yep, it is only applicable when using the prefix and suffix attributes, for example:

const signale = require('signale'); signale.complete({prefix: '[task]', message: ['Fix issue #%s', 59], suffix: '(@santa)'}); //=> [task] ☒ complete Fix issue #59 (@santa)

now I have to check undefined case.

Done. message for destructuring args

…n message when message undefined

…sue-20

klaudiosinani · 2018-06-09T15:26:09Z

signale.js

@@ -163,7 +163,7 @@ class Signale {
        [msg] = args;
      } else {
        let [{prefix, message, suffix}] = args;
-        if(message === undefined) {
+        if (message === undefined) {


Nice! It is possible to simplify it even more to a one-liner, instead of using an if statement:

msg = message ? util.format(...message) : '';

This way, in the rare case where no message is given by the user, we can pass just an empty string.

I am doing like this >> msg = message ? util.format(...message) : util.format(...args);

Hmm, if message is undefined, which means that we have no message to log, the args object will at best only hold the suffix and prefix attributes, thus they will be printed twice, as they are handled independently on lines 175 & 212, for example:

Running with: msg = message ? util.format(...message) : util.format(...args);

const signale = require('signale'); signale.complete({prefix: '[task]', suffix: '(@santa)'}); // `message` is undefined // => [task] ☒ complete { prefix: '[task]', suffix: '(@santa)' } (@santa)

Above, the printed object { prefix: '[task]', suffix: '(@santa)' } is unexpected, since we left message undefined, which means nothing should have been printed in its place

Running with: msg = message ? util.format(...message) : '';

const signale = require('signale'); signale.complete({prefix: '[task]', suffix: '(@santa)'}); // `message` is undefined // => [task] ☒ complete (@santa)

In this case, message was not defined and as it was expected nothing was printed in its the place

I edited the markdown of the above comment, so the example can look better and be more clear. Please let me know what you think about this approach : )

If i do like msg = message ? util.format(...message) : ''; then it will not print message for below example:

signale.success({a: 2}); signale.success([1, 2]);

Somehow I managed previous issue occurrence. By like this

// for fix xo warning _prefixSufixNestedTernary(prefix, suffix, args) { return prefix || suffix ? '' : util.format(args); } msg = message ? util.format(...message) : this._prefixSufixNestedTernary(prefix, suffix, ...args);

Is this fine?

Yep, you are totally right, that is an issue also described in #24. At this moment signale loggers can only accept one or more comma-delimited strings or an object containing the suffix, prefix and message attributes, which means that currently the only way to print any other object is to use template literals, which on their own are not such a perfect replacement either:

const object = {a: 2}; const array = [1, 2]; signale.success(`${object}`); //=> ✔ success [object Object] signale.success(`${array}`); //=> ✔ success 1,2

But with string interpolation it will be possible to cover these cases effectively:

signale.success('%O', {a: 2}); //=> ✔ success { a: 2 } signale.success('%O', [1, 2]); //=> ✔ success [ 1, 2 ]

Thus it is acceptable to use msg = message ? util.format(...message) : ''; in order to deliver string interpolation and also to preserve the fuctionality of signale when using the {suffix, prefix, message} object where the message attribute can be undefined. The idial case is exactly what you described, and it is something will be added as soon as possible, as a feature on its own, since it will be possible to log any object directly, in the same fashion that console.log does : )

I hope except signale.success(${object}); case other will works fine.

klaudiosinani · 2018-06-09T15:50:42Z

signale.js

@@ -162,7 +162,7 @@ class Signale {
      if (args[0] instanceof Error) {
        [msg] = args;
      } else {
-        let [{prefix, message, suffix}] = args;
+        const [{prefix, message, suffix}] = args;


Original comment: #23 (comment)

klaudiosinani · 2018-06-09T16:43:21Z

signale.js

@@ -162,7 +162,7 @@ class Signale {
      if (args[0] instanceof Error) {
        [msg] = args;
      } else {
-        let [{prefix, message, suffix}] = args;
+        const [{prefix, message, suffix}] = args;
        msg = message ? util.format(...message) : util.format(...args);


Original comment: #23 (comment)
(just in case the initial notification didn't make it through)

klaudiosinani · 2018-06-09T18:07:31Z

signale.js

@@ -155,6 +155,10 @@ class Signale {
    return meta;
  }

+  _prefixSufixNestedTernary(prefix, suffix, args) {


Awesome! A couple of minor tweaks and it is ready to be merged! 🎉

_hasAdditional({suffix, prefix}, args) { // Rename to something simpler, for example: _hasAdditional() // Use destructuring directly on function definition return (suffix || prefix) ? '' : util.format(...args); // Add simple parentheses around the condition to make it clearer }

It's better than previous

klaudiosinani · 2018-06-09T18:08:16Z

signale.js

@@ -163,7 +167,7 @@ class Signale {
        [msg] = args;
      } else {
        const [{prefix, message, suffix}] = args;
-        msg = message ? util.format(...message) : util.format(...args);
+        msg = message ? util.format(...message) : this._prefixSufixNestedTernary(prefix, suffix, ...args);
        additional = Object.assign({}, {suffix, prefix});


Just a minor reordering of the assignments:

const [{prefix, message, suffix}] = args; additional = Object.assign({}, {suffix, prefix}); msg = message ? util.format(...message) : this._hasAdditional(additional, args);

It's better than my previous method

klaudiosinani · 2018-06-09T18:21:09Z

Awesome work! Huge thank you for all your time and effort! 🎉

rjoydip added 3 commits May 27, 2018 14:21

feat(): support printf-like formatting

5731fa2

fix(test): removed one code comment

e244713

fix(klaudiosinani#24): Print stringified and colorized objects

50755f5

klaudiosinani added the enhancement New feature or request label May 27, 2018

klaudiosinani self-requested a review May 27, 2018 13:47

klaudiosinani requested changes May 27, 2018

View reviewed changes

klaudiosinani added 2 commits May 29, 2018 21:44

Merge branch 'master' into issue-20

d384e37

Merge branch 'master' into issue-20

4bee762

Merge branch 'master' into issue-20

bb02846

klaudiosinani changed the title ~~feat(): support printf-like formatting~~ Added support for string interpolation. Fixes #20 Jun 6, 2018

klaudiosinani requested changes Jun 6, 2018

View reviewed changes

rjoydip added 4 commits June 9, 2018 12:09

refactor: requested changes done but except message destructuring

9e1cac7

Merge branch 'issue-20' of https://github.com/rjoydip/signale into is…

cdbf135

…sue-20

Merge branch 'master' into issue-20

fe8c7cb

Merge branch 'master' into issue-20

5b6b670

klaudiosinani reviewed Jun 9, 2018

View reviewed changes

rjoydip added 4 commits June 9, 2018 20:40

fix: message destructuring and changed const to let for assign args i…

721a146

…n message when message undefined

Merge branch 'issue-20' of https://github.com/rjoydip/signale into is…

664013b

…sue-20

fix: lint error

938daa0

refactor: simplify to a one-liner

37a7fc1

klaudiosinani reviewed Jun 9, 2018

View reviewed changes

fix: tslint error and changed let to const

ffaa379

klaudiosinani reviewed Jun 9, 2018

View reviewed changes

rjoydip added 2 commits June 9, 2018 23:01

fix: checked suffux and prefix exists or not by func

55f58df

refactor

e7252da

klaudiosinani reviewed Jun 9, 2018

View reviewed changes

klaudiosinani approved these changes Jun 9, 2018

View reviewed changes

klaudiosinani merged commit 3d1b9c7 into klaudiosinani:master Jun 9, 2018

rjoydip-zz deleted the issue-20 branch June 9, 2018 18:24

klaudiosinani mentioned this pull request Jun 10, 2018

Does not support print symbol #42

Closed

snyk-bot mentioned this pull request Sep 5, 2019

[Snyk] Upgrade signale from 1.1.0 to 1.4.0 MailOnline/mol-conventional-changelog#18

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for string interpolation. Fixes #20 #23

Added support for string interpolation. Fixes #20 #23

rjoydip-zz commented May 27, 2018 •

edited

klaudiosinani left a comment

nick-keller commented May 29, 2018

markusberg-spt commented Jun 6, 2018

klaudiosinani commented Jun 6, 2018

klaudiosinani May 27, 2018

klaudiosinani May 27, 2018

klaudiosinani May 27, 2018

rjoydip-zz Jun 9, 2018

klaudiosinani May 27, 2018

klaudiosinani May 27, 2018

rjoydip-zz Jun 9, 2018

rjoydip-zz commented Jun 6, 2018

klaudiosinani Jun 9, 2018 •

edited

rjoydip-zz Jun 9, 2018 •

edited

klaudiosinani Jun 9, 2018 •

edited

rjoydip-zz Jun 9, 2018

rjoydip-zz Jun 9, 2018

klaudiosinani Jun 9, 2018

rjoydip-zz Jun 9, 2018

klaudiosinani Jun 9, 2018 •

edited

klaudiosinani Jun 9, 2018 •

edited

rjoydip-zz Jun 9, 2018

rjoydip-zz Jun 9, 2018

klaudiosinani Jun 9, 2018

rjoydip-zz Jun 9, 2018

klaudiosinani Jun 9, 2018

klaudiosinani Jun 9, 2018

klaudiosinani Jun 9, 2018 •

edited

rjoydip-zz Jun 9, 2018

klaudiosinani Jun 9, 2018

rjoydip-zz Jun 9, 2018

klaudiosinani commented Jun 9, 2018

Added support for string interpolation. Fixes #20 #23

Added support for string interpolation. Fixes #20 #23

Conversation

rjoydip-zz commented May 27, 2018 • edited

klaudiosinani left a comment

Choose a reason for hiding this comment

nick-keller commented May 29, 2018

markusberg-spt commented Jun 6, 2018

klaudiosinani commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjoydip-zz commented Jun 6, 2018

klaudiosinani Jun 9, 2018 • edited

Choose a reason for hiding this comment

rjoydip-zz Jun 9, 2018 • edited

Choose a reason for hiding this comment

klaudiosinani Jun 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klaudiosinani Jun 9, 2018 • edited

Choose a reason for hiding this comment

klaudiosinani Jun 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klaudiosinani Jun 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klaudiosinani commented Jun 9, 2018

rjoydip-zz commented May 27, 2018 •

edited

klaudiosinani Jun 9, 2018 •

edited

rjoydip-zz Jun 9, 2018 •

edited

klaudiosinani Jun 9, 2018 •

edited

klaudiosinani Jun 9, 2018 •

edited

klaudiosinani Jun 9, 2018 •

edited

klaudiosinani Jun 9, 2018 •

edited