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: support BigInt in util.format #22097

Closed
wants to merge 4 commits into from

Conversation

shisama
Copy link
Contributor

@shisama shisama commented Aug 3, 2018

Adding support for BigInt in util.format and console.log.
Placeholder %d is replaced as Number when BigInt is set in
second argument.
Now, util.format('%d', 1180591620717411303424n) returns
'1.1805916207174113e+21'.
However, expected result is '1180591620717411303424'.

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

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Aug 3, 2018
devsnek
devsnek previously requested changes Aug 3, 2018
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

bigint is a distinct type from number and i don't think we should mix them up. perhaps there is another letter we can use? also any output of a bigint should include the n suffix

@shisama shisama changed the title util: support BigInt in util.format [wip]util: support BigInt in util.format Aug 3, 2018
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Your codes cannot pass eslint check because over 80-character each line. You have to step up to a new line, something like this format in your test file.

assert.strictEqual(util.format('%d', 1180591620717411303424),
                   '1.1805916207174113e+21');
assert.strictEqual(util.format('%d', 1180591620717411303424n),
                   '1180591620717411303424');

@shisama shisama force-pushed the util-format-bigint branch from 28e49cd to 5dc83f4 Compare August 3, 2018 01:12
@shisama
Copy link
Contributor Author

shisama commented Aug 3, 2018

@devsnek Thanks.

perhaps there is another letter we can use?

OK, but I have no idea what the letter should be. So, Please help me if you have any ideas.
cc @nodejs/util

also any output of a bigint should include the n suffix

I agree with you. I'll fix it.

@shisama
Copy link
Contributor Author

shisama commented Aug 3, 2018

@Maledong Thanks. I fixed it.

@shisama shisama changed the title [wip]util: support BigInt in util.format util: support BigInt in util.format Aug 3, 2018
@hiroppy
Copy link
Member

hiroppy commented Aug 3, 2018

/cc @nodejs/util

doc/api/util.md Outdated
@@ -198,7 +198,7 @@ Each placeholder token is replaced with the converted value from the
corresponding argument. Supported placeholders are:

* `%s` - `String`.
* `%d` - `Number` (integer or floating point value).
* `%d` - `Number` (integer or floating point value) or `BigInt`.
Copy link
Member

Choose a reason for hiding this comment

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

If anything, shouldn't %i be more suitable for BigInt, considering it's an integer?

@silverwind
Copy link
Contributor

Before we jump to an implementation, I'd suggest first discussing it at whatwg/console. The spec says that %d and %i use parseInt for the type conversion. We are already violating this spec on %d by using Number, by the way.

@shisama
Copy link
Contributor Author

shisama commented Aug 5, 2018

@joyeecheung @silverwind Thank you for your comments. I think It's better to discuss before implementing this on Node.js. I created the issue in whatwg/console. whatwg/console#148

@silverwind
Copy link
Contributor

silverwind commented Aug 9, 2018

edit: see whatwg/console#148 (comment)

@BridgeAR BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Sep 5, 2018
Adding support for BigInt in util.format and console.log.
Placeholder `%d` is replaced as Number when BigInt is set in
second argument.
Now, `util.format('%d', 1180591620717411303424n)` returns
`'1.1805916207174113e+21'`.
However, expected result is `'1180591620717411303424'`.
@silverwind
Copy link
Contributor

Current implementation looks good to me, but please update the docs and maybe add a few more test cases with more than one specifier.

Not totally happy with the change to include the n suffix, but I see it's in line with what Chrome does.

@shisama
Copy link
Contributor Author

shisama commented Sep 18, 2018

@silverwind Thank you for your comments. I updated document util.md and add tests with two specifiers.

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, but would like to wait with landing a few more days in case there's any movement on whatwg/console#148.

@silverwind
Copy link
Contributor

@devsnek does your red cross still stand? I'd like to merge this.

@shisama
Copy link
Contributor Author

shisama commented Oct 8, 2018

@devsnek Please review again. If you request changes still, please tell me what they are.

@silverwind silverwind added semver-minor PRs that contain new features and should be released in the next minor version. and removed blocked PRs that are blocked by other issues or PRs. labels Oct 9, 2018
@silverwind
Copy link
Contributor

Thanks. Landed in d71dd97.

@silverwind silverwind closed this Oct 9, 2018
silverwind pushed a commit that referenced this pull request Oct 9, 2018
`util.format` and `console.log` now support BigInt via the existing
format specifiers `%i` and `%d`.

PR-URL: #22097
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 10, 2018
`util.format` and `console.log` now support BigInt via the existing
format specifiers `%i` and `%d`.

PR-URL: #22097
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos added a commit that referenced this pull request Oct 10, 2018
Notable changes:

* assert
  * The diff output is now a tiny bit improved by sorting object
    properties when inspecting the values that are compared with each
    other. #22788
* cli
  * The options parser now normalizes `_` to `-` in all multi-word
    command-line flags, e.g. `--no_warnings` has the same effect as
    `--no-warnings`. #23020
  * Added bash completion for the `node` binary. To generate a bash
    completion script, run `node --completion-bash`. The output can be
    saved to a file which can be sourced to enable completion.
    #20713
* crypto
  * Added support for PEM-level encryption.
    #23151
  * Added an API asymmetric key pair generation. The new methods
    `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be
    used to generate public and private key pairs. The API supports
    RSA, DSA and EC and a variety of key encodings (both PEM and DER).
    #22660
* fs
  * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If
    this option is set to true, non-existing parent folders will be
    automatically created. #21875
* http2
  * Added a `'ping'` event to `Http2Session` that is emitted whenever a
    non-ack `PING` is received.
    #23009
  * Added support for the `ORIGIN` frame.
    #22956
  * Updated nghttp2 to 1.34.0. This adds RFC 8441 extended connect
    protocol support to allow use of WebSockets over HTTP/2.
    #23284
* module
  * Added `module.createRequireFromPath(filename)`. This new method can
    be used to create a custom require function that will resolve
    modules relative to the filename path.
    #19360
* process
  * Added a `'multipleResolves'` process event that is emitted whenever
    a `Promise` is attempted to be resolved multiple times, e.g. if the
    `resolve` and `reject` functions are both called in a `Promise`
    executor. #22218
* url
  * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These
    methods can be used to correctly convert between file: URLs and
    absolute paths. #22506
* util
  * Added the `sorted` option to `util.inspect()`. If set to `true`,
    all properties of an object and Set and Map entries will be sorted
    in the returned string. If set to a function, it is used as a
    compare function. #22788
  * The `util.instpect.custom` symbol is now defined in the global
    symbol registry as `Symbol.for('nodejs.util.inspect.custom')`.
    #20857
  * Added support for `BigInt` numbers in `util.format()`.
    #22097
* V8 API
  * A number of V8 C++ APIs have been marked as deprecated since they
    have been removed in the upstream repository. Replacement APIs
    are added where necessary. #23159
* Windows
  * The Windows msi installer now provides an option to automatically
    install the tools required to build native modules.
    #22645
* Workers
  * Debugging support for Workers using the DevTools protocol has been
    implemented. #21364
  * The public `inspector` module is now enabled in Workers.
    #22769
* Added new collaborators:
  * digitalinfinity - Hitesh Kanwathirtha

PR-URL: #23313
targos added a commit that referenced this pull request Oct 10, 2018
Notable changes:

* assert
  * The diff output is now a tiny bit improved by sorting object
    properties when inspecting the values that are compared with each
    other. #22788
* cli
  * The options parser now normalizes `_` to `-` in all multi-word
    command-line flags, e.g. `--no_warnings` has the same effect as
    `--no-warnings`. #23020
  * Added bash completion for the `node` binary. To generate a bash
    completion script, run `node --completion-bash`. The output can be
    saved to a file which can be sourced to enable completion.
    #20713
* crypto
  * Added support for PEM-level encryption.
    #23151
  * Added an API asymmetric key pair generation. The new methods
    `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be
    used to generate public and private key pairs. The API supports
    RSA, DSA and EC and a variety of key encodings (both PEM and DER).
    #22660
* fs
  * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If
    this option is set to true, non-existing parent folders will be
    automatically created. #21875
* http2
  * Added a `'ping'` event to `Http2Session` that is emitted whenever a
    non-ack `PING` is received.
    #23009
  * Added support for the `ORIGIN` frame.
    #22956
  * Updated nghttp2 to 1.34.0. This adds RFC 8441 extended connect
    protocol support to allow use of WebSockets over HTTP/2.
    #23284
* module
  * Added `module.createRequireFromPath(filename)`. This new method can
    be used to create a custom require function that will resolve
    modules relative to the filename path.
    #19360
* process
  * Added a `'multipleResolves'` process event that is emitted whenever
    a `Promise` is attempted to be resolved multiple times, e.g. if the
    `resolve` and `reject` functions are both called in a `Promise`
    executor. #22218
* url
  * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These
    methods can be used to correctly convert between file: URLs and
    absolute paths. #22506
* util
  * Added the `sorted` option to `util.inspect()`. If set to `true`,
    all properties of an object and Set and Map entries will be sorted
    in the returned string. If set to a function, it is used as a
    compare function. #22788
  * The `util.instpect.custom` symbol is now defined in the global
    symbol registry as `Symbol.for('nodejs.util.inspect.custom')`.
    #20857
  * Added support for `BigInt` numbers in `util.format()`.
    #22097
* V8 API
  * A number of V8 C++ APIs have been marked as deprecated since they
    have been removed in the upstream repository. Replacement APIs
    are added where necessary. #23159
* Windows
  * The Windows msi installer now provides an option to automatically
    install the tools required to build native modules.
    #22645
* Workers
  * Debugging support for Workers using the DevTools protocol has been
    implemented. #21364
  * The public `inspector` module is now enabled in Workers.
    #22769
* Added new collaborators:
  * digitalinfinity - Hitesh Kanwathirtha

PR-URL: #23313
targos added a commit that referenced this pull request Oct 10, 2018
Notable changes:

* assert
  * The diff output is now a tiny bit improved by sorting object
    properties when inspecting the values that are compared with each
    other. #22788
* cli
  * The options parser now normalizes `_` to `-` in all multi-word
    command-line flags, e.g. `--no_warnings` has the same effect as
    `--no-warnings`. #23020
  * Added bash completion for the `node` binary. To generate a bash
    completion script, run `node --completion-bash`. The output can be
    saved to a file which can be sourced to enable completion.
    #20713
* crypto
  * Added support for PEM-level encryption.
    #23151
  * Added an API asymmetric key pair generation. The new methods
    `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be
    used to generate public and private key pairs. The API supports
    RSA, DSA and EC and a variety of key encodings (both PEM and DER).
    #22660
* fs
  * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If
    this option is set to true, non-existing parent folders will be
    automatically created. #21875
* http2
  * Added a `'ping'` event to `Http2Session` that is emitted whenever a
    non-ack `PING` is received.
    #23009
  * Added support for the `ORIGIN` frame.
    #22956
  * Updated nghttp2 to 1.34.0. This adds RFC 8441 extended connect
    protocol support to allow use of WebSockets over HTTP/2.
    #23284
* module
  * Added `module.createRequireFromPath(filename)`. This new method can
    be used to create a custom require function that will resolve
    modules relative to the filename path.
    #19360
* process
  * Added a `'multipleResolves'` process event that is emitted whenever
    a `Promise` is attempted to be resolved multiple times, e.g. if the
    `resolve` and `reject` functions are both called in a `Promise`
    executor. #22218
* url
  * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These
    methods can be used to correctly convert between file: URLs and
    absolute paths. #22506
* util
  * Added the `sorted` option to `util.inspect()`. If set to `true`,
    all properties of an object and Set and Map entries will be sorted
    in the returned string. If set to a function, it is used as a
    compare function. #22788
  * The `util.instpect.custom` symbol is now defined in the global
    symbol registry as `Symbol.for('nodejs.util.inspect.custom')`.
    #20857
  * Added support for `BigInt` numbers in `util.format()`.
    #22097
* V8 API
  * A number of V8 C++ APIs have been marked as deprecated since they
    have been removed in the upstream repository. Replacement APIs
    are added where necessary. #23159
* Windows
  * The Windows msi installer now provides an option to automatically
    install the tools required to build native modules.
    #22645
* Workers
  * Debugging support for Workers using the DevTools protocol has been
    implemented. #21364
  * The public `inspector` module is now enabled in Workers.
    #22769
* Added new collaborators:
  * digitalinfinity - Hitesh Kanwathirtha

PR-URL: #23313
jasnell pushed a commit that referenced this pull request Oct 17, 2018
`util.format` and `console.log` now support BigInt via the existing
format specifiers `%i` and `%d`.

PR-URL: #22097
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
Notable changes:

* assert
  * The diff output is now a tiny bit improved by sorting object
    properties when inspecting the values that are compared with each
    other. #22788
* cli
  * The options parser now normalizes `_` to `-` in all multi-word
    command-line flags, e.g. `--no_warnings` has the same effect as
    `--no-warnings`. #23020
  * Added bash completion for the `node` binary. To generate a bash
    completion script, run `node --completion-bash`. The output can be
    saved to a file which can be sourced to enable completion.
    #20713
* crypto
  * Added support for PEM-level encryption.
    #23151
  * Added an API asymmetric key pair generation. The new methods
    `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be
    used to generate public and private key pairs. The API supports
    RSA, DSA and EC and a variety of key encodings (both PEM and DER).
    #22660
* fs
  * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If
    this option is set to true, non-existing parent folders will be
    automatically created. #21875
* http2
  * Added a `'ping'` event to `Http2Session` that is emitted whenever a
    non-ack `PING` is received.
    #23009
  * Added support for the `ORIGIN` frame.
    #22956
  * Updated nghttp2 to 1.34.0. This adds RFC 8441 extended connect
    protocol support to allow use of WebSockets over HTTP/2.
    #23284
* module
  * Added `module.createRequireFromPath(filename)`. This new method can
    be used to create a custom require function that will resolve
    modules relative to the filename path.
    #19360
* process
  * Added a `'multipleResolves'` process event that is emitted whenever
    a `Promise` is attempted to be resolved multiple times, e.g. if the
    `resolve` and `reject` functions are both called in a `Promise`
    executor. #22218
* url
  * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These
    methods can be used to correctly convert between file: URLs and
    absolute paths. #22506
* util
  * Added the `sorted` option to `util.inspect()`. If set to `true`,
    all properties of an object and Set and Map entries will be sorted
    in the returned string. If set to a function, it is used as a
    compare function. #22788
  * The `util.instpect.custom` symbol is now defined in the global
    symbol registry as `Symbol.for('nodejs.util.inspect.custom')`.
    #20857
  * Added support for `BigInt` numbers in `util.format()`.
    #22097
* V8 API
  * A number of V8 C++ APIs have been marked as deprecated since they
    have been removed in the upstream repository. Replacement APIs
    are added where necessary. #23159
* Windows
  * The Windows msi installer now provides an option to automatically
    install the tools required to build native modules.
    #22645
* Workers
  * Debugging support for Workers using the DevTools protocol has been
    implemented. #21364
  * The public `inspector` module is now enabled in Workers.
    #22769
* Added new collaborators:
  * digitalinfinity - Hitesh Kanwathirtha

PR-URL: #23313
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.

8 participants