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

lib: add console.{group,groupCollapsed,groupEnd}() #1727

Closed
wants to merge 4 commits into from
Closed

lib: add console.{group,groupCollapsed,groupEnd}() #1727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
59d8dd8
lib: add console.{group,groupCollapsed,groupEnd}()
imyller May 18, 2015
f67438e
Improve console.markdown groupEnd() docs
imyller May 18, 2015
9a3a885
lib: improve console grouping formatting performance
imyller May 18, 2015
fd0e2f2
lib: console: small clean ups
imyller May 18, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions doc/api/console.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,21 @@ object. This is useful for inspecting large complicated objects. Defaults to
- `colors` - if `true`, then the output will be styled with ANSI color codes.
Defaults to `false`. Colors are customizable, see below.

### console.group([data][, ...])

Starts a new logging group with an optional title. All console output that occurs
after calling this method and calling `console.groupEnd` has the same
indent level.

### console.groupCollapsed([data][, ...])

Same as `console.group`.

### console.groupEnd()

Closes the most recent logging group that was created with `console.group` or
`console.groupCollapsed`.

### console.time(label)

Used to calculate the duration of a specific operation. To start a timer, call
Expand Down
38 changes: 33 additions & 5 deletions lib/console.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ function Console(stdout, stderr) {
Object.defineProperty(this, '_stderr', prop);
prop.value = new Map();
Object.defineProperty(this, '_times', prop);
prop.value = 0;
Object.defineProperty(this, '_group', prop);
prop.value = '';
Object.defineProperty(this, '_prefix', prop);

// bind the prototype functions to this Console instance
var keys = Object.keys(Console.prototype);
Expand All @@ -33,25 +37,49 @@ function Console(stdout, stderr) {
}

Console.prototype.log = function() {
this._stdout.write(util.format.apply(this, arguments) + '\n');
this._stdout.write((this._group ? util.format.apply(this, arguments).replace(
/^/gm, this._prefix) : util.format.apply(this, arguments)) + '\n');
};


Console.prototype.info = Console.prototype.log;


Console.prototype.warn = function() {
this._stderr.write(util.format.apply(this, arguments) + '\n');
this._stderr.write((this._group ? util.format.apply(this, arguments).replace(
/^/gm, this._prefix) : util.format.apply(this, arguments)) + '\n');
};


Console.prototype.error = Console.prototype.warn;


Console.prototype.dir = function(object, options) {
this._stdout.write(util.inspect(object, util._extend({
customInspect: false
}, options)) + '\n');
var str = util.inspect(object, util._extend({customInspect: false}, options));
this._stdout.write((this._group ?
str.replace(/^/gm, this._prefix) : str) + '\n');
};


Console.prototype.group = function() {
if (arguments.length) {
this.log.apply(this, arguments);
}
this._group++;
this._prefix = ' '.repeat(this._group);
};


Console.prototype.groupCollapsed = function() {
this.group.apply(this, arguments);
};


Console.prototype.groupEnd = function() {
if (this._group) {
this._group--;
this._prefix = (this._group ? ' '.repeat(this._group) : '');
Copy link
Member

Choose a reason for hiding this comment

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

Is a special case really needed here? ' '.repeat(0) === ''.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried to avoid extra function call and use single '' const that optimizes better than generating a new one for each groupEnd.

If you feel this complicates code too much, this conditional can be removed as you suggested.

Copy link

Choose a reason for hiding this comment

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

I think this is right place for this line:

if (_group` == 0) //do nothing
else //modify something.

And about:

this._prefix = (this._group ? '  '.repeat(this._group) : '');

generate some levels of indentation
or

this._prefix = this._prefix.slice(0, -2);

trim two characters from indentation level

Copy link

Choose a reason for hiding this comment

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

Oh sorry I missed that where cheched

this._group ? '  '.repeat(this._group) : ''

so if rewrite it with if statement:

if (this._group) '  '.repeat(this._group)
else {
// here is unreachable code
}

I agree with @ChALkeR

Copy link
Member Author

Choose a reason for hiding this comment

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

@3y3 So to summarize:

  • You want to get rid of the ( a ? b : c ) notation and replace with if else structures
  • Use += ' ' concatenation in group() to modify _prefix string
  • Use String.slice in groupEnd() to modify _prefix string

You have a valid approach, but here I did not choose that. Lets give the maintainers some time to comment on this and give us direction :)

Copy link

Choose a reason for hiding this comment

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

Propose this as final solution for this block:

Console.prototype.groupEnd = function() {
  if (this._group) {
    this._group--;
    this._prefix = this._prefix.slice(0, -2);
  }
}

Copy link

Choose a reason for hiding this comment

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

You want to get rid of the ( a ? b : c ) notation and replace with if else structures

No I only say, that your ( a ? b : c ) notation has unreachable part.

Look at my example.

Copy link
Member Author

Choose a reason for hiding this comment

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

 No I only say, that your ( a ? b : c ) notation has unreachable part.

Actually, It does not have unreachable part, but I do see an extra > 0 comparison that could be left out completely for optimization. Can you please define what is the unreachable part you see.

(this._group ? ' '.repeat(this._group) : ''); evaluates to '' empty string const only if _group has just been decreased to zero. Otherwise it just regenerates the _prefix with new string. It does not use slice, but it matches the code group uses for generating the prefix.

I'm just trying to avoid avoiding unnecessary string concatenations and multiple methods in functions to generate the prefix. I chose String.prototype.repeat instead of string concatenation and String.prototype.slice.

}
};


Expand Down
67 changes: 67 additions & 0 deletions test/parallel/test-console-group.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
var common = require('../common');
var assert = require('assert');

assert.ok(process.stdout.writable);
assert.ok(process.stderr.writable);
// Support legacy API
assert.equal('number', typeof process.stdout.fd);
assert.equal('number', typeof process.stderr.fd);

assert.doesNotThrow(function () {
console.group('label');
console.groupEnd();
});

assert.doesNotThrow(function () {
console.groupCollapsed('label');
console.groupEnd();
});

// test multiple calls to console.groupEnd()
assert.doesNotThrow(function () {
console.groupEnd();
console.groupEnd();
console.groupEnd();
});

var stdout_write = global.process.stdout.write;
var strings = [];
global.process.stdout.write = function(string) {
strings.push(string);
};
console._stderr = process.stdout;

// test console.group(), console.groupCollapsed() and console.groupEnd()
console.log('foo');
console.group('bar');
console.log('foo bar');
console.groupEnd();
console.groupCollapsed('group 1');
console.log('log');
console.warn('warn');
console.error('error');
console.dir({ foo: true });
console.group('group 2');
console.log('foo bar hop');
console.log('line 1\nline 2\nline 3');
console.groupEnd();
console.log('end 2');
console.groupEnd();
console.log('end 1');

global.process.stdout.write = stdout_write;

assert.equal('foo\n', strings.shift());
assert.equal('bar\n', strings.shift());
assert.equal(' foo bar\n', strings.shift());
assert.equal("group 1\n", strings.shift());
assert.equal(' log\n', strings.shift());
assert.equal(' warn\n', strings.shift());
assert.equal(' error\n', strings.shift());
assert.equal(' { foo: true }\n', strings.shift());
assert.equal(' group 2\n', strings.shift());
assert.equal(' foo bar hop\n', strings.shift());
assert.equal(' line 1\n line 2\n line 3\n', strings.shift());
assert.equal(' end 2\n', strings.shift());
assert.equal('end 1\n', strings.shift());
assert.equal(strings.length, 0);