Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
console: rework console.dirxml to call console.dir on each argument r…
…eceived.

Minimal implementation following the Living Standard specs, following
reviews.

Fixes: #17128
  • Loading branch information
Tiriel committed Nov 24, 2017
commit 6319499514475a9c91a9e279bd9af4b6246ef008
28 changes: 16 additions & 12 deletions doc/api/console.md
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,21 @@ Defaults to `2`. To make it recurse indefinitely, pass `null`.
Defaults to `false`. Colors are customizable; see
[customizing `util.inspect()` colors][].

### console.dirxml(...data)
<!-- YAML
added: v8.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/17128
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: This should be https://github.com/nodejs/node/pull/17152 as far as I can tell.

description: "`console.dirxml` now calls `console.dir` for each argument."
-->
* `...data` {any}

This method calls `console.dir()` with default options for each argument it
receives. See [`console.dir()`][] for more details about said defaults.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will require some changes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Damn, sorry about that. Fixing right now and squashing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

Please note that this method doesn't produce any xml formatting and uses the
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: Please use does not instead of the abbreviated doesn't.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: xmlXML. When in doubt, check the specification.

default `console.dir()` formatting resolution instead.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not a native English speaker myself, but resolution sounds strange here IMO (at least I never used it in this context). It is not really necessary anyway as the first sentence already has the same information.

Copy link
Copy Markdown
Contributor Author

@Tiriel Tiriel Nov 23, 2017

Choose a reason for hiding this comment

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

Well, I'll just let the precision on the absence of XML formatting then, just to be sure it's clear for everyone. But I thought resolution was indicated, you have me doubting now 😄


### console.error([data][, ...args])
<!-- YAML
added: v0.1.100
Expand Down Expand Up @@ -435,18 +450,6 @@ The following methods are exposed by the V8 engine in the general API but do
not display anything unless used in conjunction with the [inspector][]
(`--inspect` flag).

### console.dirxml(object)
<!-- YAML
added: v8.0.0
-->
* `object` {string}

This method does not display anything unless used in the inspector. The
`console.dirxml()` method displays in `stdout` an XML interactive tree
representation of the descendants of the specified `object` if possible, or the
JavaScript representation if not. Calling `console.dirxml()` on an HTML or XML
element is equivalent to calling `console.log()`.

### console.markTimeline(label)
<!-- YAML
added: v8.0.0
Expand Down Expand Up @@ -521,6 +524,7 @@ added: v8.0.0
This method does not display anything unless used in the inspector. The
`console.timelineEnd()` method is the deprecated form of [`console.timeEnd()`][].

[`console.dir()`]: #console_console_dir_obj_options
[`console.error()`]: #console_console_error_data_args
[`console.group()`]: #console_console_group_label
[`console.log()`]: #console_console_log_data_args
Expand Down
14 changes: 1 addition & 13 deletions lib/console.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,20 +163,8 @@ Console.prototype.dir = function dir(object, options) {


Console.prototype.dirxml = function dirxml(...data) {
const optionProps = ['showHidden', 'depth', 'colors'],
maybeOptions = Object.getOwnPropertyNames(data.slice(-1)),
isOption = maybeOptions.some((p) => optionProps.indexOf(p) !== -1);
let options = { customInspect: false };

if (isOption) {
options = Object.assign(data.splice(-1), options);
}
for (const item of data) {
write(this._ignoreErrors,
this._stdout,
util.inspect(item, options),
this._stdoutErrorHandler,
this[kGroupIndent]);
Console.prototype.dir.call(this, item);
}
};

Expand Down
16 changes: 16 additions & 0 deletions test/parallel/test-console.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,14 @@ console.dir(custom_inspect, { showHidden: false });
console.dir({ foo: { bar: { baz: true } } }, { depth: 0 });
console.dir({ foo: { bar: { baz: true } } }, { depth: 1 });

// test console.dirxml()
console.dirxml(custom_inspect, custom_inspect);
console.dirxml(
{ foo: { bar: { baz: true } } },
{ foo: { bar: { quux: false } } },
{ foo: { bar: { quux: true } } }
);

// test console.trace()
console.trace('This is a %j %d', { formatted: 'trace' }, 10, 'foo');

Expand Down Expand Up @@ -171,6 +179,14 @@ assert.strictEqual(strings.shift(),
"{ foo: 'bar', inspect: [Function: inspect] }\n");
assert.ok(strings.shift().includes('foo: [Object]'));
assert.strictEqual(strings.shift().includes('baz'), false);
assert.strictEqual(strings.shift(),
"{ foo: 'bar', inspect: [Function: inspect] }\n");
assert.strictEqual(strings.shift(),
"{ foo: 'bar', inspect: [Function: inspect] }\n");
assert.strictEqual(strings.shift().includes('foo: { bar: { baz:'), true);
assert.strictEqual(strings.shift().includes('quux'), true);
assert.strictEqual(strings.shift().includes('quux: true'), true);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please do not use assert.strictEqual(..., true). Just use assert(...) (or assert.ok(...)) to test for logic values. You can find the documentation here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Didn't know, sorry. Thanks!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No problem, thanks for going through our (sometimes a little long) process!


assert.ok(/^label: \d+\.\d{3}ms$/.test(strings.shift().trim()));
assert.ok(/^__proto__: \d+\.\d{3}ms$/.test(strings.shift().trim()));
assert.ok(/^constructor: \d+\.\d{3}ms$/.test(strings.shift().trim()));
Expand Down