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.

Sign up for GitHub

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

Jump to bottom

Add comment formatting #319

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

Add comment formatting #319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion lib/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
var deepEqual = require('deep-equal');
var defined = require('defined');
var path = require('path');
var format = require('util').format;
var inherits = require('inherits');
var EventEmitter = require('events').EventEmitter;
var has = require('has');
Expand Down Expand Up @@ -121,7 +122,11 @@ Test.prototype.test = function (name, opts, cb) {

Test.prototype.comment = function (msg) {
var that = this;
forEach(trim(msg).split('\n'), function (aMsg) {
// 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.

// `util.format`, however, will print stringified objects. To maintain backward compatibility
// check the args length and only call `util.format` if the invoker desires string expansion.
var message = arguments.length > 1 ? format.apply(null, arguments) : msg;
forEach(trim(message).split('\n'), function (aMsg) {
that.emit('result', trim(aMsg).replace(/^#\s*/, ''));
});
};
Expand Down
4 changes: 2 additions & 2 deletions readme.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,9 +277,9 @@ after `t` will not be run until all subtests finish.

You may pass the same options that [`test()`](#testname-opts-cb) accepts.

## t.comment(message)
## t.comment(message[, ...])

Print a message without breaking the tap output. (Useful when using e.g. `tap-colorize` where output is buffered & `console.log` will print in incorrect order vis-a-vis tap output.)
Print a message without breaking the tap output. Accepts optional args for `util.format`-style formatting. Useful when using e.g. `tap-colorize` where output is buffered & `console.log` will print in incorrect order vis-a-vis tap output.

## var htest = test.createHarness()

Expand Down
54 changes: 54 additions & 0 deletions test/comment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,3 +173,57 @@ tap.test('multiline string', function (assert) {
t.end();
});
});

tap.test('formatted string', function (assert) {
assert.plan(1);

var verify = function (output) {
assert.equal(output.toString('utf8'), [
'TAP version 13',
'# formatted string',
'# tip tap tape',
'',
'1..0',
'# tests 0',
'# pass 0',
'',
'# ok',
''
].join('\n'));
};

var test = tape.createHarness();
test.createStream().pipe(concat(verify));
test('formatted string', function (t) {
t.comment("tip %s t%s", "tap", "ape");
t.end();
});
});

tap.test('formatted multiline string', function (assert) {
assert.plan(1);

var verify = function (output) {
assert.equal(output.toString('utf8'), [
'TAP version 13',
'# formatted multiline string',
'# tip',
'# tap',
'# tape',
'',
'1..0',
'# tests 0',
'# pass 0',
'',
'# ok',
''
].join('\n'));
};

var test = tape.createHarness();
test.createStream().pipe(concat(verify));
test('formatted multiline string', function (t) {
t.comment("tip\n%s\nt%s", "tap", "ape");
t.end();
});
});