Add comment formatting #319
Conversation
lib/test.js
Outdated
| var that = this; | ||
| var keys = Object.keys(arguments) | ||
| var msg = keys.length > 1 ? format.apply(null, arguments) : arguments[keys[0]]; |
There was a problem hiding this comment.
- just
arguments.lengthis sufficient, no need toObject.keys(also that makes it require ES5, which this lib does not) - It seems like this might be easier to just
format.apply(null, arguments)in every case?
There was a problem hiding this comment.
Calling format.apply in all cases changes the behavior, e.g. test.comment({answer:42}) will print {answer:42} whereas the current code prints [object Object]. I'm happy to make the change, as I think that's preferable behavior, but it's up to you.
There was a problem hiding this comment.
Ah, interesting. We definitely want to avoid breaking changes.
Either way, I think perhaps the ES3 equivalent of format.apply(null, Array.prototype.map.call(arguments, String)) might be the semantic we want here? What's the use case for supporting format's behavior with non-strings?
There was a problem hiding this comment.
Use case would be, e.g., console.log-style debugging where you want to log object contents. With the change as it stands you can do t.cmt('%j', obj). Formatting everything, you could do t.cmt(obj).
Doesn't sound like 5 fewer characters is worth the risk of breaking older code tho.
|
@ljharb Pushed fixes. |
lib/test.js
Outdated
| @@ -106,8 +107,11 @@ Test.prototype.test = function (name, opts, cb) { | |||
| }); | |||
| }; | |||
|
|
|||
| Test.prototype.comment = function (msg) { | |||
| Test.prototype.comment = function () { | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
0x1mason
force-pushed
the
master
branch
3 times, most recently
from
4d63927
to
c76157b
Compare
| @@ -108,6 +109,12 @@ Test.prototype.test = function (name, opts, cb) { | |||
|
|
|||
| Test.prototype.comment = function (msg) { | |||
| var that = this; | |||
|
|
|||
| // Previous behavior involved `toString` calls on objects, i.e. emitting `[object Object]`. | |||
There was a problem hiding this comment.
@ljharb Added a comment to explain the conditional
There was a problem hiding this comment.
sadly I still don't think this is sufficient - previously, multiple arguments were ignored, and I still might have done arr.forEach(t.comment) for example, and relied on the stringification despite passing 3 arguments. I think that the first argument must never be stringified to avoid a breaking change.
There was a problem hiding this comment.
Are you suggesting something like this?
if args[0] is a string and args.len > 1:
expand the string
else:
ignore
There was a problem hiding this comment.
Also, what about a new function ,eg format, comments, etc
There was a problem hiding this comment.
No, I'm suggesting always stringifying the first argument no matter what, and not adding the ability to implicitly log object contents.
There was a problem hiding this comment.
This remains an issue - to restate, I think that we need something like format.apply(null, Array.prototype.map.call(arguments, String)) to ensure that everything is always stringified.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ljharb
added
semver-minor: new stuff
This PR allows for
util.formatstyle comment formatting