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

Add comment formatting #319

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@0x1mason
Copy link

0x1mason commented Sep 7, 2016

This PR allows for util.format style comment formatting

lib/test.js Outdated
var that = this;
var keys = Object.keys(arguments)
var msg = keys.length > 1 ? format.apply(null, arguments) : arguments[keys[0]];

This comment has been minimized.

@ljharb

ljharb Sep 7, 2016

Collaborator
  • just arguments.length is sufficient, no need to Object.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?

This comment has been minimized.

@0x1mason

0x1mason Sep 8, 2016

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.

This comment has been minimized.

@ljharb

ljharb Sep 8, 2016

Collaborator

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?

This comment has been minimized.

@0x1mason

0x1mason Sep 8, 2016

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.

@0x1mason

This comment has been minimized.

Copy link

0x1mason commented Sep 8, 2016

@ljharb Pushed fixes.

@0x1mason 0x1mason force-pushed the 0x1mason:master branch from 9710991 to 50d740e Sep 8, 2016

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.

@ljharb

ljharb Sep 8, 2016

Collaborator

i think it'd still be good to leave message here tho, so the function's .length is correct :-)

This comment was marked as resolved.

@0x1mason

@0x1mason 0x1mason force-pushed the 0x1mason:master branch 3 times, most recently from 4d63927 to c76157b Sep 8, 2016

@@ -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]`.

This comment has been minimized.

@0x1mason

0x1mason Sep 8, 2016

@ljharb Added a comment to explain the conditional

This comment has been minimized.

@ljharb

ljharb Sep 8, 2016

Collaborator

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.

This comment has been minimized.

@0x1mason

0x1mason Sep 8, 2016

Are you suggesting something like this?

if args[0] is a string and args.len > 1:
  expand the string
else:
  ignore

This comment has been minimized.

@0x1mason

0x1mason Sep 8, 2016

Also, what about a new function ,eg format, comments, etc

This comment has been minimized.

@ljharb

ljharb Sep 8, 2016

Collaborator

No, I'm suggesting always stringifying the first argument no matter what, and not adding the ability to implicitly log object contents.

This comment has been minimized.

@ljharb

ljharb Jan 9, 2019

Collaborator

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.

@ljharb

This comment was marked as resolved.

Copy link
Collaborator

ljharb commented Jan 9, 2019

@0x1mason could you check the "allow edits" checkbox on the right hand side of the PR?

@0x1mason

This comment was marked as resolved.

Copy link

0x1mason commented Jan 9, 2019

@ljharb done

@ljharb ljharb force-pushed the 0x1mason:master branch from c76157b to 5e86f30 Jan 9, 2019

@ljharb
Copy link
Collaborator

ljharb left a comment

This has been rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment