util: add %i and %f formatting specifiers by silverwind · Pull Request #10308 · nodejs/node

silverwind · 2016-12-16T20:36:57Z

Checklist

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

Affected core subsystem(s)

util

Description of change

This change brings formatting specifiers available in util.format and consequently, console.* closer to what is supported in all major browsers. There is a breaking change with the %d specifier which previously served a double purpose of formatting both integer and floats. With this change, it will format only as integer.

%d is being changed to format only as integer.
%i is introduced as an alias to %d.
%f is introduced to format floating point values.

When updating code, all instances of %d format strings that were supplied with floats should be changed to the %f format string.

Fixes: #10292

Fishrock123 · 2016-12-16T21:21:30Z

I think we should just keep the behavior of %d the same. I suspect widespread ecosystem dependence.

cjihrig · 2016-12-16T21:32:00Z

I agree with @Fishrock123 about %d. It would also be nice if we could make this code a little more DRY. The code for %d, %i, %s, and %j is almost exactly the same.

silverwind · 2016-12-16T21:33:30Z

I'd wager it wouldn't break much, in the end it's mostly just used in console.

@thealphanerd can we get a CITGM run on this?
@cjihrig agree, I can see about refactoring that.

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

Fishrock123 · 2016-12-17T05:32:41Z

I would prefer %d to be Number, %i to be Integer-ish, and %f to be Float-ish.

sam-github · 2016-12-19T16:22:39Z

I don't think we should bring node's routines to write to stdout closer in-line with the browser debug log routines, even if for historical reasons they have the same name, console.

If this is really an improvement, it should be compelling on its own.

silverwind · 2016-12-19T21:35:13Z

@cjihrig @mscdex let me know what you think about the DRY refactoring in 1277352. Performance is very slightly degraded it seems. I tried with both string and charcode object lookup, the string version seems to win because of the implicit toString that happens with number properties.

util/inspect.js n=5000000: 73042.29909012877 (before)
util/inspect.js n=5000000: 72325.39568358353 (string lookup)
util/inspect.js n=5000000: 71563.77710267795 (charcode lookup)

mscdex · 2016-12-19T21:54:05Z

@silverwind Why the refactor in the first place?

silverwind · 2016-12-19T22:00:39Z

@mscdex it was requested above because of all the duplicate code.

cjihrig · 2016-12-20T02:43:39Z

Did you happen to try leaving the switch statement, and just having fall through cases, then calling out to formatters?

jasnell · 2016-12-23T06:35:58Z

hmm.. while I'm generally ok with this, I think semver-major is more appropriate given the change in behavior for %d

jasnell

LGTM as a semver-major

mscdex · 2016-12-23T14:31:14Z

Personally I would still like to see the object lookup replaced by a switch or something to avoid the perf hit.

silverwind · 2016-12-23T16:30:17Z

@jasnell there's no behavorial change in the current version, the change to %d was backed out upon request. There's been some discussion surrounding a new WHATWG console spec in #10292, and I've asked for a CTC decision on whether we should follow that spec. I'd certainly would welcome a %d that behaves just like in browsers.

jasnell · 2016-12-24T00:09:13Z

Assuming an actual console spec emerges, I'd be +1 on ensuring that we are compliant to it. My apologies, I hadn't noticed that the changes to %d were backed out. As long as there are no behavioral changes then, +1 to semver-minor

jasnell · 2017-03-24T23:04:40Z

Updates on this one?

TimothyGu · 2017-03-24T23:07:21Z

A list of standardized formatting specifiers: https://console.spec.whatwg.org/#formatting-specifiers

mscdex · 2017-03-24T23:29:01Z

FWIW I'm still concerned about performance regressions, especially with the object lookup.

This change brings formatting specifiers available in `util.format` and consequently, `console.*` closer to what is supported in all major browsers. There is a breaking change with the `%d` specifier which previously served a double purpose of formatting both integer and floats. With this change, it will format only as integer. - `%d` is being changed to format only as integer. - `%i` is introduced as an alias to `%d`. - `%f` is introduced to format floating point values. When updating code, all instances of `%d` format strings that were supplied with floats should be changed to the `%f` format string. Fixes: nodejs#10292

Fishrock123 · 2017-03-29T14:52:06Z

belated lgtm

This change brings formatting specifiers available in `util.format` and consequently, `console.*` closer to what is supported in all major browsers. - `%i` is introduced to format integer values. - `%f` is introduced to format floating point values. Fixes: nodejs#10292 PR-URL: nodejs#10308 Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * util: console is now closer to what is supported in all major browsers (Roman Reiss) [#10308](#10308) PR-URL: #12319

Notable changes: * util: console is now closer to what is supported in all major browsers (Roman Reiss) [nodejs#10308](nodejs#10308) PR-URL: nodejs#12319

Notable changes: * util: console is now closer to what is supported in all major browsers (Roman Reiss) [#10308](nodejs/node#10308) PR-URL: nodejs/node#12319 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Notable changes: * util: console is now closer to what is supported in all major browsers (Roman Reiss) [#10308](#10308) PR-URL: #12319

gibfahn · 2018-01-15T21:25:06Z

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

This change brings formatting specifiers available in `util.format` and consequently, `console.*` closer to what is supported in all major browsers. - `%i` is introduced to format integer values. - `%f` is introduced to format floating point values. Fixes: #10292 PR-URL: #10308 Reviewed-By: James M Snell <jasnell@gmail.com>

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

This LTS release comes with 112 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 30 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

This LTS release comes with 112 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 30 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) nodejs#12678 * crypto: - expose ECDH class (Bryan English) nodejs#8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) nodejs#10209 - warn on invalid authentication tag length (Tobias Nießen) nodejs#17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) nodejs#16835 * dgram: - added socket.setMulticastInterface() (Will Young) nodejs#7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) nodejs#13005 * lib: - return this from net.Socket.end() (Sam Roberts) nodejs#13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) nodejs#16386 * net: - return this from getConnections() (Sam Roberts) nodejs#13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) nodejs#13784 * repl: - improve require() autocompletion (Alexey Orlenko) nodejs#14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) nodejs#16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) nodejs#12087 - add process.ppid (cjihrig) nodejs#16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) nodejs#12839 * tools, build: - a new macOS installer! (JP Wesselink) nodejs#15179 * url: - WHATWG URL api support (James M Snell) nodejs#7448 * util: - add %i and %f formatting specifiers (Roman Reiss) nodejs#10308 PR-URL: nodejs#18342

nodejs-github-bot added util Issues and PRs related to the built-in util module. lts-watch-v6.x labels Dec 16, 2016

silverwind force-pushed the util-format-substitutions branch from 2b356ad to c337c64 Compare December 16, 2016 20:46

nodejs-github-bot added the lts-watch-v6.x label Dec 16, 2016

silverwind added semver-major PRs that contain breaking changes and should be released in the next major version. console Issues and PRs related to the console subsystem. and removed lts-watch-v6.x labels Dec 16, 2016

silverwind added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Dec 19, 2016

nodejs-github-bot added the lts-watch-v6.x label Dec 19, 2016

jasnell approved these changes Dec 23, 2016

View reviewed changes

jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017

silverwind added 2 commits March 25, 2017 11:23

change %d to format number

bde7402

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

italoacasas mentioned this pull request Apr 10, 2017

v7.9.0 Release Proposal #12319

Merged

2 tasks

italoacasas pushed a commit that referenced this pull request Apr 11, 2017

2017-04-11, Version 7.9.0 (Current)

0794478

Notable changes: * util: console is now closer to what is supported in all major browsers (Roman Reiss) [#10308](#10308) PR-URL: #12319

italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 11, 2017

2017-04-11, Version 7.9.0 (Current)

b8f4160

Notable changes: * util: console is now closer to what is supported in all major browsers (Roman Reiss) [nodejs#10308](nodejs#10308) PR-URL: nodejs#12319

cpojer mentioned this pull request Apr 13, 2017

console.log doesn't interpret %c format option. jestjs/jest#3010

Closed

gibfahn mentioned this pull request Apr 24, 2017

Potential Semver Minor Backports nodejs/Release#188

Closed

31 tasks

evanlucas pushed a commit that referenced this pull request Apr 25, 2017

2017-04-11, Version 7.9.0 (Current)

7db0af6

Notable changes: * util: console is now closer to what is supported in all major browsers (Roman Reiss) [#10308](#10308) PR-URL: #12319

gibfahn mentioned this pull request Jun 7, 2017

Review semver-minor backports nodejs/Release#228

Closed

8 tasks

MylesBorins mentioned this pull request Jan 24, 2018

v6.13.0 proposal #18342

Merged

Uh oh!

Conversation

silverwind commented Dec 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Fishrock123 commented Dec 16, 2016

Uh oh!

cjihrig commented Dec 16, 2016

Uh oh!

silverwind commented Dec 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fishrock123 commented Dec 17, 2016

Uh oh!

sam-github commented Dec 19, 2016

Uh oh!

silverwind commented Dec 19, 2016

Uh oh!

mscdex commented Dec 19, 2016

Uh oh!

silverwind commented Dec 19, 2016

Uh oh!

cjihrig commented Dec 20, 2016

Uh oh!

jasnell commented Dec 23, 2016

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

mscdex commented Dec 23, 2016

Uh oh!

silverwind commented Dec 23, 2016

Uh oh!

jasnell commented Dec 24, 2016

Uh oh!

jasnell commented Mar 24, 2017

Uh oh!

TimothyGu commented Mar 24, 2017

Uh oh!

mscdex commented Mar 24, 2017

Uh oh!

Fishrock123 commented Mar 29, 2017

Uh oh!

gibfahn commented Jan 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

silverwind commented Dec 16, 2016 •

edited

Loading

silverwind commented Dec 16, 2016 •

edited

Loading