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

Add comment formatting #319

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

0x1mason
Copy link

@0x1mason 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]];
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • 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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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
Copy link
Author

0x1mason commented Sep 8, 2016

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

@0x1mason 0x1mason force-pushed the master branch 3 times, most recently from 4d63927 to c76157b Compare September 8, 2016 13:07
@@ -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]`.
Copy link
Author

Choose a reason for hiding this comment

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

@ljharb Added a comment to explain the conditional

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Are you suggesting something like this?

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 has been minimized.

@ljharb ljharb added the needs "allow edits" PR needs "allow edits" checked label Jan 9, 2019
@0x1mason

This comment has been minimized.

@ljharb ljharb removed the needs "allow edits" PR needs "allow edits" checked label Jan 9, 2019
Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This has been rebased.

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

Successfully merging this pull request may close these issues.

2 participants