console: improve console.group() by Trott · Pull Request #14999 · nodejs/node

Trott · 2017-08-24T03:39:17Z

Preserve indentation for multiline strings, objects that span multiple
lines, etc.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

console

/cc @BridgeAR

Trott · 2017-08-24T03:47:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9818/

BridgeAR · 2017-08-24T03:57:36Z

lib/console.js

You can also check for \n first by adding a indexOf guard. Using indexOf for cases were there is no newline it is a lot faster and almost negligible in case there is one.

if (groupIndent.length !== 0 && string.indexOf("\n") !== -1) { // ...

(Yes, indexOf is still faster then includes)

I'd have to move the prepending of groupIndent if I did that so the result would probably be:

if (groupIndent.length !== 0) { if (string.indexOf('\n') !== -1 { string = string.replace(/\n/g, `\n${groupIndent}`); } string = groupIndent + string; } string += '\n';

...or...

if (groupIndent.length !== 0 && string.indexOf("\n") !== -1) { string = string.replace(/\n/g, `\n${groupIndent}`); } string = groupIndent + string + '\n';

I would do the latter while I would actually not replace the string again but simply place it like this

if (!ignoreErrors) return stream.write(`${groupIndent}${string}\n`);

(I went with the first option in my above comment.)

BridgeAR

A nit but otherwise LGTM

BridgeAR · 2017-08-24T04:02:19Z

@Trott just a thought - what do you think about hiding and protecting the Symbol a bit?

{
    writable: true,
    enumerable: false,
    configurable: false
}

TimothyGu

The reason why I didn't suggest this on the original PR is that we can go down the rabbit hole real quick with this kind of changes. For one, I can see a legitimate argument to handle '\r\n' and even '\r'. If we do handle them, we might need to adjust column width etc. for modules that are e.g. curses work-alikes using \r. And the list goes on.

I'd prefer to keep group() minimal.

BridgeAR · 2017-08-24T05:09:13Z

@TimothyGu I would suggest to keep the implementation similar to the ones used in browsers. I do not think we should support \r and this would also break the indentation of not user supplied line breaks but also from util.inspect that is called depending on the input to console.log.

Trott · 2017-08-24T05:15:20Z

@TimothyGu I agree that there's a rabbit-hole danger, but this seems pretty important IMO considering console.log(objectThatGetsDisplayedMultiline).

Trott · 2017-08-24T05:17:19Z

@Trott just a thought - what do you think about hiding and protecting the Symbol a bit?

@BridgeAR Done.

Preserve indentation for multiline strings, objects that span multiple lines, etc.

Hide the internal `groupIndent` key a bit by making it non-enumerable and non-configurable.

jasnell

Better. thank you

Trott · 2017-08-24T05:27:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9819/

hiroppy

👍

Preserve indentation for multiline strings, objects that span multiple lines, etc. also make groupIndent non-enumerable Hide the internal `groupIndent` key a bit by making it non-enumerable and non-configurable. PR-URL: #14999 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

jasnell · 2017-08-25T18:36:31Z

Landed in af11867

Preserve indentation for multiline strings, objects that span multiple lines, etc. also make groupIndent non-enumerable Hide the internal `groupIndent` key a bit by making it non-enumerable and non-configurable. PR-URL: nodejs/node#14999 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

MylesBorins · 2017-09-10T02:53:52Z

Setting as don't land as console.group doesn't exist on v8.x

@Trott please change label if neccessary

Trott · 2017-09-10T15:50:57Z

@Trott please change label if neccessary

Changed! :-D

Preserve indentation for multiline strings, objects that span multiple lines, etc. also make groupIndent non-enumerable Hide the internal `groupIndent` key a bit by making it non-enumerable and non-configurable. PR-URL: #14999 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

MylesBorins · 2017-09-20T21:03:05Z

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

Trott added the console Issues and PRs related to the console subsystem. label Aug 24, 2017

Trott mentioned this pull request Aug 24, 2017

console: implement minimal console.group() #14910

Closed

4 tasks

BridgeAR reviewed Aug 24, 2017

View reviewed changes

BridgeAR approved these changes Aug 24, 2017

View reviewed changes

TimothyGu reviewed Aug 24, 2017

View reviewed changes

Trott added 2 commits August 23, 2017 22:22

console: improve console.group()

483dec7

Preserve indentation for multiline strings, objects that span multiple lines, etc.

console: make groupIndent non-enumerable

9df027b

Hide the internal `groupIndent` key a bit by making it non-enumerable and non-configurable.

Trott force-pushed the console-group-known-issues branch from 8858cc8 to 9df027b Compare August 24, 2017 05:22

TimothyGu approved these changes Aug 24, 2017

View reviewed changes

BridgeAR approved these changes Aug 24, 2017

View reviewed changes

jasnell approved these changes Aug 24, 2017

View reviewed changes

lpinca approved these changes Aug 24, 2017

View reviewed changes

watilde approved these changes Aug 24, 2017

View reviewed changes

hiroppy approved these changes Aug 25, 2017

View reviewed changes

jasnell closed this Aug 25, 2017

MylesBorins added the dont-land-on-v8.x label Sep 10, 2017

Trott removed the dont-land-on-v8.x label Sep 10, 2017

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the dont-land-on-v6.x label Sep 20, 2017

Trott deleted the console-group-known-issues branch January 13, 2022 22:46

Uh oh!

Conversation

Trott commented Aug 24, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Trott commented Aug 24, 2017

Uh oh!

BridgeAR Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Aug 24, 2017

Uh oh!

TimothyGu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Aug 24, 2017

Uh oh!

Trott commented Aug 24, 2017

Uh oh!

Trott commented Aug 24, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 24, 2017

Uh oh!

hiroppy left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 25, 2017

Uh oh!

MylesBorins commented Sep 10, 2017

Uh oh!

Trott commented Sep 10, 2017

Uh oh!

MylesBorins commented Sep 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

TimothyGu left a comment •

edited

Loading