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.

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

util: implement %o as formatting specifier #14558

Closed
wants to merge 5 commits into from

Conversation

gla5001
Copy link
Contributor

@gla5001 gla5001 commented Jul 31, 2017

Implementing the %o and %O as formatting specifier for util.format.
Based on discussion in issue, this specifier should just call
util.inspect to format passed in value.

Fixes: #14545

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)

util

Implementing the %o and %O as formatting specifier for util.format.
Based on discussion in issue, this specifier should just call
util.inspect to format passed in value.

Fixes: nodejs#14545
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Jul 31, 2017
@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt
Copy link
Contributor

One ubuntu1604-arm64 failure in the CI seems unrelated.

@gla5001 Thank you for your contribution!

@Trott
Copy link
Member

Trott commented Aug 1, 2017

/cc @Fishrock123

@tniessen tniessen self-assigned this Aug 1, 2017
@jasnell
Copy link
Member

jasnell commented Aug 1, 2017

This may possibly also need documentation added (I cannot remember how extensively we've documented the formatting specifiers)

@gla5001
Copy link
Contributor Author

gla5001 commented Aug 1, 2017

Yup. Overlooked that. I'll update docs

Copy link
Contributor

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

LGTM, except one nit.

doc/api/util.md Outdated
@@ -167,6 +167,9 @@ corresponding argument. Supported placeholders are:
* `%f` - Floating point value.
* `%j` - JSON. Replaced with the string `'[Circular]'` if the argument
contains circular references.
* `%o` - Object. A string representation of an object.
Similar to `util.inspect()` without options.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Indent this line

@silverwind silverwind added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 1, 2017
@refack
Copy link
Contributor

refack commented Aug 1, 2017

Is it too late to consider %o to be JSON.stringify(obj, null, ' ') which is just pretty JSON.
I feel like making %o and %O do the same is a missed opportunity...

@refack
Copy link
Contributor

refack commented Aug 1, 2017

Or if there's no consensus let's implement just one, since changing it later will be semver-major

@silverwind
Copy link
Contributor

silverwind commented Aug 1, 2017

I think it'd make sense to indent %o, it certainly fits the "optimally useful formatting" mentioned in the spec. Maybe with two spaces as that's the most common form of indentation in JS.

@gla5001
Copy link
Contributor Author

gla5001 commented Aug 1, 2017

So im happy to implement whatever. Just need a consensus on it.

  1. Keep as is
  2. Remove the %O and just have %o
  3. Change %o to JSON.stringify with 2 spaces

@refack
Copy link
Contributor

refack commented Aug 1, 2017

Quoting #14545 (comment)

%O

An object with generic JavaScript object formatting is a potentially expandable representation of a generic JavaScript object.

%o

An object with optimally useful formatting is an implementation-specific, potentially-interactive representation of an object judged to be maximally useful and informative.


IMHO
%o - JSON.stringify(obj, null, ' ')
%O - util.inspect(obj)

@Fishrock123
Copy link
Contributor

Fishrock123 commented Aug 1, 2017

I don't agree with @refack here. I don't think that JSON-ifying is more detailed, rather it is less, as JSON can't represent everything.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Aug 1, 2017

Considering browsers differentiate this by using a util.inspect()-like output with variances that are not possible in Node, I think it backs that up.

Nit fix and changing behavior of %o
@gla5001
Copy link
Contributor Author

gla5001 commented Aug 1, 2017

Should they be the same then? By JSON-ifying one and inspecting the other, you now the option of displaying more or less detail. Which could be useful in some cases?

doc/api/util.md Outdated
@@ -167,6 +167,11 @@ corresponding argument. Supported placeholders are:
* `%f` - Floating point value.
* `%j` - JSON. Replaced with the string `'[Circular]'` if the argument
contains circular references.
* `%o` - Object. A string representation of an object
with optimally useful formatting.
* `%O` - Object. A string representation of an object
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we should state explicitly that JSON stringification is involved here with its restrictions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vsemozhetbyt Hows that look?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems explicit enough if we do have a consensus for this behavior.

@refack
Copy link
Contributor

refack commented Aug 1, 2017

I don't agree with @refack here. I don't think that JSON-ifying is more detailed, rather it is less, as JSON can't represent everything.

@Fishrock123 So are you suggesting to reverse may suggestion for %O and %o? I'm fine with that, it's more important to me that have get two more representations. Or are you suggesting not to use JSON.stringify at all?

so tl;dr in terms of "human usefulness" %j < %O < %o ?

> const nestedObj = {
...   foo: 'bar',
...   foobar: {
.....     foo: 'bar',
.....     func: function() {}
.....   }
... };

> console.log("%j", nestedObj)
{"foo":"bar","foobar":{"foo":"bar"}}

> console.log(JSON.stringify(nestedObj, null, '  '))
{
  "foo": "bar",
  "foobar": {
    "foo": "bar"
  }
}

> console.log(util.inspect(nestedObj))
{ foo: 'bar', foobar: { foo: 'bar', func: [Function: func] } }

or go big and

> console.log(util.inspect(nestedObj, {showHidden: true, breakLength:10}))
{ foo: 'bar',
  foobar:
   { foo: 'bar',
     func:
      { [Function: func]
        [length]: 0,
        [name]: 'func',
        [prototype]: [Object] } } }

BTW: Chrome 60 has no obvius distinction
image
Nither does FF 54
image

@refack
Copy link
Contributor

refack commented Aug 1, 2017

Another idea ( @gla5001 sorry for the noise ), add %J so:
%j - JSON.stringify(obj)
%J - JSON.stringify(obj, null, ' ')
%O - util.inspect(obj)
%o - util.inspect(obj, {showHidden: true, breakLength:10})

@tniessen
Copy link
Member

tniessen commented Aug 4, 2017

Landed in 211df3f! Thank you for your efforts, great first contribution! 🎉

@tniessen tniessen closed this Aug 4, 2017
tniessen pushed a commit that referenced this pull request Aug 4, 2017
Implementing the %o and %O formatting specifiers for util.format.
Based on discussion in issue, this specifier should just call
util.inspect to format the value.

PR-URL: #14558
Fixes: #14545
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@gla5001
Copy link
Contributor Author

gla5001 commented Aug 4, 2017

awesome. thanks!

@tniessen
Copy link
Member

tniessen commented Aug 4, 2017

Just a few tips for the future:

  • You might want to configure git to warn you about whitespace problems, e.g. trailing whitespace. We fix those problems when landing your changes, but it can make rebasing more difficult. You can also configure some editors to highlight those problems, and depending on your git interface, you might see such problems highlighted in red when reviewing your changes.
  • Our commit message guidelines do not apply to fixup commits, so you don't need to repeat the same title throughout all commits within a PR, as long as the first commit has a proper message. While landing PRs, we usually squash your commits, and having the same commit title each time is not helpful. For fixup commits, just describe what you fixed in the commit title. (This does not apply to PRs which consist of multiple independent commits, but most PRs are a single commit and then a lot of fixups.)

We are looking forward to your next contribution!

@refack
Copy link
Contributor

refack commented Aug 4, 2017

@gla5001 congrats on GitHub promoting you from:
image
to
image
🍾

@refack
Copy link
Contributor

refack commented Aug 4, 2017

P.S. a nice second contribution might be adding an indent option to util.inspect 😉

@gla5001
Copy link
Contributor Author

gla5001 commented Aug 4, 2017

@tniessen Thanks! I'll make sure to take those tips into account next time.

@refack Happy to look at that one next. Is there an issue for it?

@refack
Copy link
Contributor

refack commented Aug 4, 2017

@refack Happy to look at that one next. Is there an issue for it?

No issue just my comment above #14558 (comment)

@domenic
Copy link
Contributor

domenic commented Aug 4, 2017

I would not suggest that as a PR, as I doubt it's something the community is interested in (I personally think it's a bad idea). It would need a dedicated issue with discussion and appropriate input from decider-people.

@refack
Copy link
Contributor

refack commented Aug 4, 2017

Well @gla5001 opening an issue and discussing a new feature is also a nice contribution.

@gla5001
Copy link
Contributor Author

gla5001 commented Aug 5, 2017

Starting discussion #14638

addaleax pushed a commit that referenced this pull request Aug 10, 2017
Implementing the %o and %O formatting specifiers for util.format.
Based on discussion in issue, this specifier should just call
util.inspect to format the value.

PR-URL: #14558
Fixes: #14545
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Aug 13, 2017
addaleax added a commit that referenced this pull request Aug 13, 2017
Ref: #14558
PR-URL: #14810
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax added a commit that referenced this pull request Aug 13, 2017
Ref: #14558
PR-URL: #14810
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@addaleax addaleax mentioned this pull request Aug 13, 2017
addaleax added a commit that referenced this pull request Aug 13, 2017
Notable changes

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](#8837)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](#14558)

PR-URL: #14811
addaleax added a commit that referenced this pull request Aug 14, 2017
Notable changes

* **HTTP2**
  * Experimental support for the built-in `http2` has been added via the
    `--expose-http2` flag.
    [#14239](#14239)

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](#8837)
  * Multiple contexts, as created by the `vm` module, are supported now.
    [#14465](#14465)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](#14558)

PR-URL: #14811
evanlucas pushed a commit that referenced this pull request Aug 15, 2017
Notable changes

* **HTTP2**
  * Experimental support for the built-in `http2` has been added via the
    `--expose-http2` flag.
    [#14239](#14239)

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](#8837)
  * Multiple contexts, as created by the `vm` module, are supported now.
    [#14465](#14465)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](#14558)

PR-URL: #14811
MSLaguana pushed a commit to nodejs/node-chakracore that referenced this pull request Aug 21, 2017
Notable changes

* **HTTP2**
  * Experimental support for the built-in `http2` has been added via the
    `--expose-http2` flag.
    [#14239](nodejs/node#14239)

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](nodejs/node#8837)
  * Multiple contexts, as created by the `vm` module, are supported now.
    [#14465](nodejs/node#14465)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](nodejs/node#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](nodejs/node#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](nodejs/node#14558)

PR-URL: nodejs/node#14811
@gibfahn
Copy link
Member

gibfahn commented Jan 15, 2018

Release team were +1 on backporting to v6.x.

@wltsmrz wltsmrz mentioned this pull request Apr 24, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

console,util: implement %o as formatting specifier