-
-
Notifications
You must be signed in to change notification settings - Fork 230
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 string interpolation. Fixes #20 #23
Changes from 13 commits
5731fa2
e244713
50755f5
d384e37
4bee762
bb02846
9e1cac7
cdbf135
fe8c7cb
5b6b670
721a146
664013b
938daa0
37a7fc1
ffaa379
55f58df
e7252da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,4 +12,4 @@ yarn.lock | |
.vscode | ||
.idea | ||
*.swp | ||
*.swo | ||
*.swo |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
'use strict'; | ||
const util = require('util'); | ||
const path = require('path'); | ||
const chalk = require('chalk'); | ||
const figures = require('figures'); | ||
|
@@ -161,12 +162,15 @@ class Signale { | |
if (args[0] instanceof Error) { | ||
[msg] = args; | ||
} else { | ||
const [{prefix, message, suffix}] = args; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks nice! The only thing left is to bring back 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 : ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that I have to do. I am getting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, it is only applicable when using the const signale = require('signale');
signale.complete({prefix: '[task]', message: ['Fix issue #%s', 59], suffix: '(@santa)'});
//=> [task] ☒ complete Fix issue #59 (@santa) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. now I have to check undefined case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
msg = message; | ||
let [{prefix, message, suffix}] = args; | ||
if (message === undefined) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! It is possible to simplify it even more to a one-liner, instead of using an 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am doing like this >> There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, if
const signale = require('signale');
signale.complete({prefix: '[task]', suffix: '(@santa)'}); // `message` is undefined
// => [task] ☒ complete { prefix: '[task]', suffix: '(@santa)' } (@santa) Above, the printed object
const signale = require('signale');
signale.complete({prefix: '[task]', suffix: '(@santa)'}); // `message` is undefined
// => [task] ☒ complete (@santa) In this case, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 : ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If i do like signale.success({a: 2});
signale.success([1, 2]); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hope except |
||
message = args; | ||
} | ||
msg = util.format(util.inspect(...message)); | ||
additional = Object.assign({}, {suffix, prefix}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's better than my previous method |
||
} | ||
} else { | ||
msg = args.join(' '); | ||
msg = util.format(...args); | ||
} | ||
|
||
const signale = this._meta(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here
message
is needed for destructuringargs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 }
Can you give some use cases so tat i can check that.