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: add sorted option #22788

Closed
wants to merge 5 commits into from
Closed

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 10, 2018

The first commit adds the ordered sorted option to util.inspect(). It makes sure object inspection output is sorted and stable, no matter of the property insertion order or the order in the set / map.

The second commit improves the assertion diff output by using the ordered sorted option. If requested, I'll move this PR out in a second one that relies on this.

Refs: #22763

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

@BridgeAR BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 10, 2018
@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. util Issues and PRs related to the built-in util module. labels Sep 10, 2018
@targos
Copy link
Member

targos commented Sep 10, 2018

I'd call it "sorted"

@mscdex
Copy link
Contributor

mscdex commented Sep 10, 2018

What does 'reliable' mean here exactly? To me that sounds like 'stable' which we don't have yet for sorting in V8 (but will soon as this behavior has been merged into V8 recently FWIW).

@BridgeAR
Copy link
Member Author

I have no strong opinion about either ordered, sorted or stable as name.

@mscdex I used stable in the commit description. If the sort is truly stable (as in the new V8 version) does not matter for this use case since all values are strings.

@BridgeAR
Copy link
Member Author

Small poll @nodejs/util for finding the best name

👍 ordered
🎉 sorted
❤️ stable

@jdalton
Copy link
Member

jdalton commented Sep 12, 2018

When I saw the issue title and began to review the PR I thought this was an option to preserve object property order (I thought maybe the existing implementation may have messed that up and this was a back compat way to fix it) and not something dealing with sorting. I was surprised when I got to the code bit and it was for sorting properties.

@BridgeAR BridgeAR changed the title util: add ordered option util: add sorted option Sep 12, 2018
@BridgeAR
Copy link
Member Author

I just renamed it to sorted.

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

lib/util.js Outdated
if (ctx.sorted === true) {
output = output.sort();
return reduceToSingleString(ctx, output.sort(), base, braces);
}
Copy link
Member

@jdalton jdalton Sep 12, 2018

Choose a reason for hiding this comment

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

☝️ This is sorting things twice. I believe the first one, output = output.sort(), isn't needed.

lib/util.js Outdated
return reduceToSingleString(ctx, output.sort(), base, braces);
}
return reduceToSingleString(ctx, output.sort(ctx.sorted), base, braces);
}
return reduceToSingleString(ctx, output, base, braces);
Copy link
Member

Choose a reason for hiding this comment

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

👆 To dry out the condition above it could be something like:

const comparator = ctx.sorted === true ? undefined : ctx.sorted
return reduceToSingleString(ctx, output.sort(comparator), base, braces)

@BridgeAR BridgeAR force-pushed the improve-assertion-error branch from 140bce8 to c979552 Compare September 12, 2018 22:41
@BridgeAR
Copy link
Member Author

Comments addressed :)

@BridgeAR
Copy link
Member Author

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 12, 2018
@BridgeAR BridgeAR force-pushed the improve-assertion-error branch from c979552 to 314a7f8 Compare September 13, 2018 09:20
@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 13, 2018

Rebased due to conflicts.

New CI https://ci.nodejs.org/job/node-test-pull-request/17168/ (:negative_squared_cross_mark:)

doc/api/util.md Outdated
* `sorted` {boolean|Function} If specified, the output for all object types
besides arrays and typed arrays is going to be stable. If set to `true` the
default sort is going to be used. If set to a function, it is used as
[compare function][].
Copy link
Member

Choose a reason for hiding this comment

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

as a [compare function][]?

doc/api/util.md Outdated
@@ -426,6 +429,10 @@ changes:
objects the same as arrays. Note that no text will be reduced below 16
characters, no matter the `breakLength` size. For more information, see the
example below. **Default:** `true`.
* `sorted` {boolean|Function} If specified, the output for all object types
besides arrays and typed arrays is going to be stable. If set to `true` the
Copy link
Member

Choose a reason for hiding this comment

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

I’m not sure it’s obvious what stable means? Maybe just explain what happens, i.e. that entries will be sorted before being printed?

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 14, 2018
The order option can be used to sort the inspected values in case
they do not rely on their order as arrays. That way the output is
stable no matter of the object property inspection order.
@BridgeAR BridgeAR force-pushed the improve-assertion-error branch from 314a7f8 to 036759e Compare September 17, 2018 08:34
@BridgeAR
Copy link
Member Author

PTAL. I just rebased due to conflicts and addressed the comments. I also added an example and made sure that non-index properties on arrays and typed arrays will also be sorted. The changed part is in the fixup: util commit.

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

doc/api/util.md Outdated
@@ -535,6 +542,34 @@ console.log(inspect(weakSet, { showHidden: true }));
// WeakSet { { a: 1 }, { b: 2 } }
```

The `sorted` option makes sure the output is stable, no matter of the properties
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 the word stable is a bit confusing here. Compare this sentence with the wiki definition (referenced from the MDN Array.prototype.sort() page):

Stable sort algorithms sort identical elements in the same order that they appear in the input.

I.e. the traditional meaning of the stable sorting is opposite to no matter of the properties insertion order.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, I think I found a better wording.

The output is now a tiny bit improved by sorting object properties
when inspecting the values that are compared with each other. That
reduces the overall diff for identical objects with a different
property insertion order.
targos pushed a commit that referenced this pull request Sep 24, 2018
The order option can be used to sort the inspected values in case
they do not rely on their order as arrays. That way the output is
stable no matter of the object property inspection order.

Backport-PR-URL: #23039
PR-URL: #22788
Refs: #22763
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos added backported-to-v10.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v10.x labels Sep 24, 2018
oyyd pushed a commit to oyyd/node that referenced this pull request Sep 25, 2018
The output is now a tiny bit improved by sorting object properties
when inspecting the values that are compared with each other. That
reduces the overall diff for identical objects with a different
property insertion order.

PR-URL: nodejs#22788
Refs: nodejs#22763
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@oyyd oyyd mentioned this pull request Sep 26, 2018
2 tasks
BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 3, 2018
The output is now a tiny bit improved by sorting object properties
when inspecting the values that are compared with each other. That
reduces the overall diff for identical objects with a different
property insertion order.

PR-URL: nodejs#22788
Refs: nodejs#22763
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Oct 5, 2018
The output is now a tiny bit improved by sorting object properties
when inspecting the values that are compared with each other. That
reduces the overall diff for identical objects with a different
property insertion order.

Backport-PR-URL: #23226
PR-URL: #22788
Refs: #22763
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Oct 7, 2018
The output is now a tiny bit improved by sorting object properties
when inspecting the values that are compared with each other. That
reduces the overall diff for identical objects with a different
property insertion order.

Backport-PR-URL: #23226
PR-URL: #22788
Refs: #22763
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos added a commit that referenced this pull request Oct 7, 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
* 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
* **Windows**
  * The Windows msi installer now provides an option to automatically
    install the tools required to build native modules.
    #22645
* **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
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
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
@BridgeAR BridgeAR deleted the improve-assertion-error branch January 20, 2020 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. 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