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

console: add table method #18137

Closed
wants to merge 1 commit into
base: master
from

Conversation

@devsnek
Member

devsnek commented Jan 13, 2018

Refs https://console.spec.whatwg.org/#table

new super fine console.table method

i've been using firefox as my model for how the outputs should look

some column ordering stuff:

  • indexed properties before named properties unless properties is passed
  • Values column is always last, independent from properties
  • Key column always directly before Values column, independent from properties
  • (index) and (iteration index) columns are always first, independent from properties
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

console

@Trott

Thanks for doing this. Can you update the documentation for console.table() in console.md?

@@ -249,6 +249,75 @@ Console.prototype.groupEnd = function groupEnd() {
this[kGroupIndent].slice(0, this[kGroupIndent].length - 2);
};
const untablableTypes = [
'symbol', 'undefined', 'boolean', 'number', 'string', 'function',

This comment has been minimized.

@TimothyGu

TimothyGu Jan 14, 2018

Member

function works just fine in Chrome: console.table([function a() {}, function b() {}]).

];
const untablableValues = [
undefined, null, NaN, true, false, global, process,

This comment has been minimized.

@TimothyGu

TimothyGu Jan 14, 2018

Member

Why are global and process singled out?

This comment has been minimized.

@devsnek

devsnek Feb 1, 2018

Member

they are objectively easier to read when not run through the table method, and the spec says Fall back to just logging the argument if it can’t be parsed as tabular. which i think is fairly true for both of those.

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

I agree in this case that it is going to be easier to read. The question is if we want to have a special handling here. And there could theoretically be other things that are difficult to read as well because they have lots of properties.

In chrome window works as tableable and I think that is comparable. I guess most people will not add process or global so I would say either we should not special handle them at all or we add a limit that by checking the property number.

if (isSet(O))
return Array.from({ length: O.size }, (e, i) => i);
if (isSetIterator(O))
return Array.from(O).map((i) => i - 1);

This comment has been minimized.

@TimothyGu

TimothyGu Jan 14, 2018

Member

This will exhaust O, preventing it from further usage; use the previewMapIterator and previewSetIterator from internal/v8 instead. Make sure to set a limit in how many elements to allow. See how it's used in util.format (the limit is 100 in this case):

node/lib/util.js

Lines 821 to 840 in c84582c

function formatCollectionIterator(preview, ctx, value, recurseTimes,
visibleKeys, keys) {
var nextRecurseTimes = recurseTimes === null ? null : recurseTimes - 1;
var vals = preview(value, 100);
var output = [];
for (const o of vals) {
output.push(formatValue(ctx, o, nextRecurseTimes));
}
return output;
}
function formatMapIterator(ctx, value, recurseTimes, visibleKeys, keys) {
return formatCollectionIterator(previewMapIterator, ctx, value, recurseTimes,
visibleKeys, keys);
}
function formatSetIterator(ctx, value, recurseTimes, visibleKeys, keys) {
return formatCollectionIterator(previewSetIterator, ctx, value, recurseTimes,
visibleKeys, keys);
}

if (isSetIterator(O))
return Array.from(O).map((i) => i - 1);
if (isMap(O))
O = O.keys();

This comment has been minimized.

@TimothyGu

TimothyGu Jan 14, 2018

Member

Did you mean return [...O.keys()];?

if (isMap(O))
O = O.keys();
if (isMapIterator(O))
return Array.from(O);

This comment has been minimized.

@TimothyGu

TimothyGu Jan 14, 2018

Member

Same as SetIterator

untablableValues.includes(value) ||
untablableTypes.includes(typeof value);
const { isSet, isMap, isSetIterator, isMapIterator } = process.binding('util');

This comment has been minimized.

@TimothyGu

TimothyGu Jan 14, 2018

Member

Move to the top

assert.strictEqual(queue.shift(), `(index) a b
0 1 undefined
1 undefined 2
`);

This comment has been minimized.

@TimothyGu

TimothyGu Jan 14, 2018

Member

Needs more tests:

console.table([3, 4, { a: 3 }]);
console.table([3, 4, { a: 3 }], ['nonexistent']);
console.table([3, 4, { a: 3 }], ['Value']);
console.table([3, 4, { Value: 3 }], ['Value']);
console.table([,,,,,,,,,4]);
console.table([[,,,,,,,,,4]]);
console.table([[,,,,,,,,,{}]]);

// etc.

// Check browsers for sensible behaviors.

Also needed to test are Map objects, Map Iterator objects, Set objects, Set Iterator objects.

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Feb 1, 2018

Ping @devsnek

@devsnek

This comment has been minimized.

Member

devsnek commented Feb 1, 2018

yeah sorry i've been meaning to get to this, its on my list

@devsnek

This comment has been minimized.

Member

devsnek commented Feb 1, 2018

@Trott @TimothyGu changes have been made

@Trott Trott dismissed their stale review Feb 2, 2018

requested changes have been made

@TimothyGu TimothyGu self-assigned this Feb 3, 2018

```
```js
console.log([{ a: 1, b: 'Y' }, { a: 'Z', b: 2 }], ['a']);

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

I guess console.log should actually be console.table.

@@ -249,6 +252,74 @@ Console.prototype.groupEnd = function groupEnd() {
this[kGroupIndent].slice(0, this[kGroupIndent].length - 2);
};
const untablableTypes = [

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

Shall we maybe call it untableable?

This comment has been minimized.

@devsnek

devsnek Feb 17, 2018

Member

doesn't really make sense, the method below is already named that

This comment has been minimized.

@BridgeAR

BridgeAR Feb 17, 2018

Member

You misunderstood my comment: I did not mean the whole variable name but only the beginning. And that for all of these variables.

The difference is the e.

This comment has been minimized.

@devsnek

devsnek Feb 17, 2018

Member

oh. i have no idea. pretty sure tableable/tablable isn't english anyway so i have no idea where to go with that. i think following other words it would become tablible but that looks really wrong.

This comment has been minimized.

@BridgeAR

BridgeAR Mar 2, 2018

Member

Maybe @Trott has a opinion about it?

];
const untablableValues = [
undefined, null, NaN, true, false, global, process,

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

I agree in this case that it is going to be easier to read. The question is if we want to have a special handling here. And there could theoretically be other things that are difficult to read as well because they have lots of properties.

In chrome window works as tableable and I think that is comparable. I guess most people will not add process or global so I would say either we should not special handle them at all or we add a limit that by checking the property number.

function getTablableKeysOf(O) {
if (isSet(O))
return Array.from({ length: O.size }, (e, i) => i);

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

The spec says it should log the properties. We could show those and add a special handling for them but e.g. Chrome does not show them.

The same applies to the entries below.

else
item = item[column];
}
r += `\t${item}`;

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

This will always print \tundefined for Map and Set but it is possible to get the value from those.

for (const column of columns) {
let item = data[row];
if (deep) {
if (isMap(item))

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

This can probably never be reached. The same applies to isSet.

2 3
`);
test([3, 4, { a: 3 }], ['nonexistent'], `(index) a

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

Chrome shows this as the following that I think would be great to have:

(index) Value a
0 3
1 4
2   3

This comment has been minimized.

@devsnek

devsnek Feb 17, 2018

Member

i don't understand the output of that table

This comment has been minimized.

@BridgeAR

BridgeAR Feb 17, 2018

Member

There is no undefined value for a in index 0 and 1. The value does not exist. But there is a value and we can show that with 3 and 4.

Just enter console.table([3, 4, { a: 3 }]) in chrome to see the output :-)

This comment has been minimized.

@devsnek

devsnek Feb 17, 2018

Member

rn the only disparity seems to be that i choose to ignore filters of props that don't exist while the browsers use undefined. i can change that.

This comment has been minimized.

@BridgeAR

BridgeAR Feb 17, 2018

Member

This is how it looks like for me:

image
image

This comment has been minimized.

@TimothyGu

TimothyGu Feb 17, 2018

Member

@BridgeAR There isn’t really a standard on what should be printed. As long as the output makes sense, which IMO it does, it’s good.

This comment has been minimized.

@BridgeAR

BridgeAR Feb 18, 2018

Member

@TimothyGu I am aware that there is no standard for it but I feel like the output used in chrome does make more sense and provides a relatively good user experience.

This comment has been minimized.

@apapirovski

apapirovski Mar 2, 2018

Member

I'm in favour of the change requested by @BridgeAR here, in relation to missing vs undefined values. I think it would go a long way towards making this feature more useful.

test([{ a: 1 }, { b: 2 }], `(index) a b
0 1 undefined
1 undefined 2

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

I think it would be best to add extra padding up to the value length that has the highest length of the column. That way it will improve the readability massively.

This comment has been minimized.

@devsnek

devsnek Feb 17, 2018

Member

i was considering that but i decided that as this isn't really intended to do more than humor the spec, and in general tables in terminals have a history of being formatted with single tabs without compensation (especially in configs and stuff) so i wanted to stay consistent with that.

This comment has been minimized.

@BridgeAR

BridgeAR Feb 17, 2018

Member

I feel like it improves the readability a lot, so I would really like to add that. I do not see any issues about inconsistencies here.

This comment has been minimized.

@BridgeAR

BridgeAR Mar 2, 2018

Member

Showing the value undefined is actually wrong here out of my perspective because there is just no value. So it should be empty instead of showing undefined. Otherwise this can not be distinguished from the value undefined.

As an extra test I would add:

test(
  [{ a: 1 }, { b: undefined }], 
  '(index) a b' +
  '0       1 ' +
  '1         undefined'
)
if (isSet(O))
return Array.from({ length: O.size }, (e, i) => i);
if (isSetIterator(O))
return previewSetIterator(O, 50);

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

I feel like we should handle iterators as not tableable.

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Feb 16, 2018

Ping @devsnek

'symbol', 'undefined', 'boolean', 'number', 'string',
];
const untablableValues = [undefined, null, NaN, true, false];

This comment has been minimized.

@BridgeAR

BridgeAR Mar 2, 2018

Member

The booleans, undefined and NaN are actually redundant to the untablableTypes. So it would be best to remove the untablableValues and just explicitely test for null in the untableable function.

@@ -249,6 +252,74 @@ Console.prototype.groupEnd = function groupEnd() {
this[kGroupIndent].slice(0, this[kGroupIndent].length - 2);
};
const untablableTypes = [

This comment has been minimized.

@BridgeAR

BridgeAR Mar 2, 2018

Member

Maybe @Trott has a opinion about it?

const untablableValues = [undefined, null, NaN, true, false];
const untablable = (value) =>
Object.keys(value).length > 20 ||

This comment has been minimized.

@BridgeAR

BridgeAR Mar 2, 2018

Member

This check has to be last. Otherwise it will fail for null and undefined. Please add a test for this as well.

const item = data[row];
if (!item || (typeof item !== 'object' && typeof item !== 'function'))
continue;
for (const key of getTablableKeysOf(data[row])) {

This comment has been minimized.

@BridgeAR

BridgeAR Mar 2, 2018

Member

data[row] should be replaced with item here.

test([{ a: 1 }, { b: 2 }], `(index) a b
0 1 undefined
1 undefined 2

This comment has been minimized.

@BridgeAR

BridgeAR Mar 2, 2018

Member

Showing the value undefined is actually wrong here out of my perspective because there is just no value. So it should be empty instead of showing undefined. Otherwise this can not be distinguished from the value undefined.

As an extra test I would add:

test(
  [{ a: 1 }, { b: undefined }], 
  '(index) a b' +
  '0       1 ' +
  '1         undefined'
)
@devsnek

This comment has been minimized.

Member

devsnek commented Mar 3, 2018

@BridgeAR new code is here, i'm planning to clean up cli_table.js a bit but the behavior is where i want it to be

@devsnek devsnek added the in progress label Mar 3, 2018

@@ -249,6 +253,86 @@ Console.prototype.groupEnd = function groupEnd() {
this[kGroupIndent].slice(0, this[kGroupIndent].length - 2);
};
const untablable = (value) =>
value == null ||
['symbol', 'undefined', 'boolean', 'number', 'string']

This comment has been minimized.

@BridgeAR

BridgeAR Mar 4, 2018

Member

undefined is redundant to value == null. Either the check above should be strict equal or the undefined should be removed here.

@devsnek devsnek removed the in progress label Mar 4, 2018

@BridgeAR

Even though I have a lot of comments, I am pretty happy what direction this currently takes!

I feel this is going to be much nicer than originally anticipated.

Some of my comments require adding new tests as well to make sure this really works as expected.

<!-- eslint-skip -->
```js
// these can't be parsed as tabular data

This comment has been minimized.

@BridgeAR

BridgeAR Mar 4, 2018

Member

Nit: Please use upper case letters as first character of a comment :-)

├─────────┼─────┼─────┤
01'Y'
1'Z'2
└─────────┴─────┴─────┘

This comment has been minimized.

@BridgeAR

BridgeAR Mar 4, 2018

Member

I suggest to visualize the output as comment as in:

// ┌─────────┬─────┬─────┐
// │ (index) │  a  │  b  │
// ├─────────┼─────┼─────┤
// │    0    │  1  │ 'Y' │
// │    1    │ 'Z' │  2  │
// └─────────┴─────┴─────┘

I guess in that case disabling eslint is also not necessary anymore.

const ITKEY = '(iteration index)';
const inspect = (v) => {
const opt = { depth: 0, maxArrayLength: 3 };

This comment has been minimized.

@BridgeAR

BridgeAR Mar 4, 2018

Member

Nit: The options do not have to be recreated on each call though and could be in the outer scope. Since the function itself works sync, it is safe to override the depths on each call. Using options is also not necessary in case the value is a primitive.

This comment has been minimized.

@devsnek

devsnek Mar 4, 2018

Member

i'd rather create them inline instead of resetting a shared object on each call, is it really needed? nvm i misunderstood, will change

┌───────────────────────────────┐
│ a │
├───────────────────────────────┤
│ [ 1, 2, 3, ... 3 more items ] │

This comment has been minimized.

@BridgeAR

BridgeAR Mar 4, 2018

Member

I think it would be better to output this as:

(index)  0  1   2   3   4 ...  length
a           1   2   3   4 ...  6
const util = require('util');
const kCounts = Symbol('counts');
const {
keys: ObjectKeys,

This comment has been minimized.

@BridgeAR

BridgeAR Mar 4, 2018

Member

I suggest to use Object.getOwnPropertyNames. That way we will for example also list the length property for arrays.

This comment has been minimized.

@devsnek

devsnek Mar 4, 2018

Member

none of the browser show length but i wouldn't be totally apposed to it if someone else brings this up

This comment has been minimized.

@BridgeAR

This comment has been minimized.

@BridgeAR

BridgeAR Mar 4, 2018

Member

That is chromium ;-)

const cell = row[i];
const len = countSymbols(cell);
const needed = (columnWidths[i] - len) / 2;
out += `${' '.repeat(needed)}${cell}${' '.repeat(Math.ceil(needed))}`;

This comment has been minimized.

@BridgeAR

BridgeAR Mar 4, 2018

Member

This is inconsistent: please move the Math.ceil call to the upper line so needed is already the ceil value. Otherwise the first repeat might be called with a floating point number. Or is that intended behavior? In that case a comment would be good.

This comment has been minimized.

@devsnek

devsnek Mar 4, 2018

Member

one is ceil and the other is normal rounding, so that when we have floating results we end up always pushing the element to the leftmost center instead of rightmost center

This comment has been minimized.

@BridgeAR

BridgeAR Mar 5, 2018

Member

Can you please still add a comment here?

for (var j = 0; j < longestColumn; j++) {
if (!rows[j])
rows[j] = [];
const v = rows[j][i] = column.hasOwnProperty(j) ? column[j] : '';

This comment has been minimized.

@BridgeAR

BridgeAR Mar 4, 2018

Member

I am not sure I understand the necessity for hasOwnProperty here. Can you elaborate?

This comment has been minimized.

@devsnek

devsnek Mar 4, 2018

Member

columns may not be filled all the way to the bottom, or may have empty slots, this replaces them with an empty string for rendering (otherwise they will have that "undefined" you asked me to remove)

bottom_right: '',
bottom_left: '',
bottom_middle: '',
right_middle: '',

This comment has been minimized.

@BridgeAR

BridgeAR Mar 4, 2018

Member

Can we please stick to our "normal" variable names and use camelCase instead of snake_case?

TableChars.middle_middle.repeat(i + 2));
let final = `${TableChars.top_left}${
divider.join(TableChars.top_middle)}${TableChars.top_right}\n`;

This comment has been minimized.

@BridgeAR

BridgeAR Mar 4, 2018

Member

Please prevent using multi line template strings and concat the string instead if necessary.
This is especially easy here as that is already often the case for final. I would also go ahead and remove final += ... and just do:

let final = 'foo' +
  'bar' +
  'baz';
for (const row of rows)
  final += row;
const divider = columnWidths.map((i) =>
TableChars.middle_middle.repeat(i + 2));
let final = `${TableChars.top_left}${

This comment has been minimized.

@BridgeAR

BridgeAR Mar 4, 2018

Member

Suggestion: rename this to result?

@TimothyGu TimothyGu removed their assignment Mar 5, 2018

@BridgeAR

Nice progress so far. After looking at it again I think it is best to make everything coherent to either be like an Array or like an Object.

'[\\u001B\\u009B][[\\]()#;?]*(?:(?:(?:[a-zA-Z\\d]*',
'(?:;[a-zA-Z\\d]*)*)?\\u0007)',
'(?:(?:\\d{1,4}(?:;\\d{0,4})*)?[\\dA-PRZcf-ntqry=><~]))',
].join('|'), 'g');

This comment has been minimized.

@BridgeAR

BridgeAR Mar 5, 2018

Member

This seems to be a super complex regular expression and we actually have a function to remove colors already. See

function removeColors(str) {
Does this solve something that this function does not?

└───────────────────┴──────────┴────────┘
`);
test(new Set([1, 2, Symbol()]), `

This comment has been minimized.

@BridgeAR

BridgeAR Mar 5, 2018

Member

Can you add a test with extra properties on the Set? The same for Map and Array. As in:

const set = new Set()
set.foo = 'bar'
│ (index) │ a │
├─────────┼───────────┤
│ a │ undefined │
│ b │ undefined │

This comment has been minimized.

@BridgeAR

BridgeAR Mar 5, 2018

Member

This should be empty and not undefined.

We might also consider always showing the Values as it is not possible to limit the output to those and it is hard to tell if the user wants to see them or not.

This comment has been minimized.

@devsnek

devsnek Mar 8, 2018

Member

assuming there's no Number.prototype.a, this looks correct

This comment has been minimized.

@BridgeAR

BridgeAR Mar 9, 2018

Member

I am not sure I understand your comment. Why should it show undefined if there is no undefined value?

└─────────┴────────┘
`);
test({ a: undefined }, ['x'], `

This comment has been minimized.

@BridgeAR

BridgeAR Mar 5, 2018

Member

This should probably be changed to: { a: { x: undefined } }, ['x']

if (tabularData == null ||
(typeof tabularData !== 'object' && typeof tabularData !== 'function') ||
(!isArray(tabularData) && ObjectKeys(tabularData) > 20))

This comment has been minimized.

@BridgeAR

BridgeAR Mar 5, 2018

Member

While looking into this deeper: limiting keys here is not necessary. It is necessary though for nested objects / functions and that is not done currently. It would also be good to add a test for this.

(!isArray(tabularData) && ObjectKeys(tabularData) > 20))
return this.log(tabularData);
const final = (k, v) => this.log(cliTable(k, v));

This comment has been minimized.

@BridgeAR

BridgeAR Mar 5, 2018

Member

Why the overhead of recreating a function on each call if it only has to be created once?

This comment has been minimized.

@devsnek

devsnek Mar 5, 2018

Member

like this?

const finTable = (c, k, v) => c.log(cliTable(k, v));
if (mapIter)
tabularData = previewMapIterator(tabularData);
if (mapIter || isMap(tabularData)) {

This comment has been minimized.

@BridgeAR

BridgeAR Mar 5, 2018

Member

I suggest to have a identical behavior for Set, SetIteraror, Array, TypedArray.
All other types should be handled identical as Object.

The only difference necessary is the name for the index. That way the output becomes way more coherent.

valuesKeyArray[i] = inspect(item);
} else {
const keys = properties || ObjectKeys(item);
for (const key of keys) {

This comment has been minimized.

@BridgeAR

BridgeAR Mar 5, 2018

Member

Suggestion: add a special case in case no keys exist from the item and to show the item in the Values in that case.

This comment has been minimized.

@devsnek

devsnek Mar 5, 2018

Member

i assume you mean for non-arrays and non-objects?

This comment has been minimized.

@BridgeAR

BridgeAR Mar 5, 2018

Member

No, I mean e.g. an empty array ([]) or empty object ({}) etc. Primitives are already handled above, so there is no point for those. That way it this will be visible instead of being invisible.

Another point: this will not work with Set / Map / SetIterator / MapIterator.

if (properties === undefined && (item === null ||
(typeof item !== 'function' && typeof item !== 'object'))) {
hasPrimitives = true;
valuesKeyArray[i] = inspect(item);

This comment has been minimized.

@BridgeAR

BridgeAR Mar 5, 2018

Member

Would you be so kind and add a comment why a holey array is created here and below? (I see the point of it but it makes it easier when you read it for the first time to understand what is going on)

Console.prototype.table = function(tabularData, properties) {
if (properties !== undefined && !ArrayIsArray(properties)) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'properties',
'array', properties);

This comment has been minimized.

@BridgeAR

BridgeAR Mar 5, 2018

Member

Please use Array here instead of array.

@BridgeAR BridgeAR self-assigned this Mar 5, 2018

> console.table(Symbol())
Symbol()
> console.table(undefined)
undefined

This comment has been minimized.

@BridgeAR

BridgeAR Mar 5, 2018

Member

Can you please comment the output? That is always done in in other places to mark output :-)

You can also write e.g., "Prints:" in-front of it if you like.

That way the eslint comment can also be removed.

-->
* `tabularData` {any}
* `properties` {Array<String>} Alternate properties for constructing the table.

This comment has been minimized.

@TimothyGu

TimothyGu Mar 9, 2018

Member

string[]

test(new Map([ ['a', 1], [Symbol(), [2]] ]), `
┌───────────────────┬──────────┬────────┐
│ (iteration index) │ Key │ Values │

This comment has been minimized.

@TimothyGu

TimothyGu Mar 9, 2018

Member

nit: Keys

This comment has been minimized.

@BridgeAR

BridgeAR Mar 9, 2018

Member

Actually: we might want to change Values to Value instead. Because it is a single value at the specific index position.

@BridgeAR

I do not want to block this any further from landing. So LGTM. I would like to get some further adjustments in but I can open a PR for that at some point in the future.

@devsnek

This comment has been minimized.

@devsnek

This comment has been minimized.

@devsnek

This comment has been minimized.

Member

devsnek commented Mar 30, 2018

as a heads-up i will be landing this in a few hours

@devsnek

This comment has been minimized.

Member

devsnek commented Mar 31, 2018

landed in 97ace04

@devsnek devsnek closed this Mar 31, 2018

@devsnek devsnek deleted the devsnek:feature/console-table branch Mar 31, 2018

devsnek added a commit that referenced this pull request Mar 31, 2018

console: add table method
PR-URL: #18137
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

@targos targos added semver-minor and removed author ready labels Apr 2, 2018

@targos

This comment has been minimized.

Member

targos commented Apr 2, 2018

Don't forget the semver-minor label for PRs that add new features ;)

@targos

This comment has been minimized.

Member

targos commented Apr 2, 2018

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

devsnek added a commit to devsnek/node that referenced this pull request Apr 2, 2018

console: add table method
PR-URL: nodejs#18137
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@vitaly-t

This comment has been minimized.

Contributor

vitaly-t commented Apr 24, 2018

Method console.table has been in Node.js for a very long time, but it was doing nothing. And now that there is an implementation, it raises a question of how to do a proper check that the current version of Node.js actually implements console.table?

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Apr 24, 2018

@vitaly-t I can think of two ways: 1) check for the version. Everything from 10.x on does something. It will likely be backported as well. So that might not be a reliable check. 2) Hook into stdout while a module is loaded and do a feature check.

BridgeAR added a commit to BridgeAR/node that referenced this pull request May 1, 2018

console: add table method
PR-URL: nodejs#18137
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

@vsemozhetbyt vsemozhetbyt referenced this pull request May 4, 2018

Closed

console: console.table() uses colored inspect #20510

2 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment