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: improve inspect compact #26269

Closed
wants to merge 3 commits into from

Conversation

BridgeAR
Copy link
Member

Please check the commit messages for a closer description.
The tests visualize the differences.

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
Copy link
Collaborator

@BridgeAR sadly an error occured when I tried to trigger a build :(

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Feb 22, 2019
doc/api/util.md Outdated Show resolved Hide resolved
doc/api/util.md Outdated Show resolved Hide resolved
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Docs and tests LGTM, but I’m having a hard time reviewing the code itself

@BridgeAR
Copy link
Member Author

@addaleax I'll add a couple of comments to the code. I hope that'll help? And is there a specific part with which you struggled most / something that I should definitely explain?

@addaleax
Copy link
Member

@BridgeAR Tbh, I’m having trouble with the string-building parts of the util.inspect() code in general?

I think comments would be great, but comments with examples of how the calculation plays out in the end would be the most helpful thing here…?

@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 25, 2019

@addaleax I'll open another commit to further improve the documentation of the string-building part.

I just pushed another commit that includes a lot of documentation, further tests and it also improves the grouping logic further and makes sure array grouping and line combining does not happen at the same time.

@BridgeAR
Copy link
Member Author

@jasnell @addaleax @nodejs/documentation PTAL. I pushed some further code.

@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 28, 2019

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 28, 2019
@BridgeAR
Copy link
Member Author

@addaleax @jasnell would you be so kind and confirm your LG due to the recent changes?

@Trott @vsemozhetbyt it would be great if either of you could also review this.

lib/internal/util/inspect.js Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Feb 28, 2019

Documentation and tests LGTM.

This overloads the `compact` option from `util.inspect()`. If it's
set to a number, it is going to align all most inner entries on the
same lign if they adhere to the following:

* The entries do not exceed the `breakLength` options value.
* The entry is one of the local most inner levels up the the one
  provided in `compact`.
When using `util.inspect()` with `compact` mode set to a number, all
array entries exceeding 6 are going to be grouped together into
logical parts.
@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 28, 2019

Rebased due to conflicts (besides that there were no changes, just the typo fixed).

CI https://ci.nodejs.org/job/node-test-pull-request/21060/ ✔️

@BridgeAR BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 28, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Feb 28, 2019
This overloads the `compact` option from `util.inspect()`. If it's
set to a number, it is going to align all most inner entries on the
same lign if they adhere to the following:

* The entries do not exceed the `breakLength` options value.
* The entry is one of the local most inner levels up the the one
  provided in `compact`.

PR-URL: nodejs#26269
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Feb 28, 2019
When using `util.inspect()` with `compact` mode set to a number, all
array entries exceeding 6 are going to be grouped together into
logical parts.

PR-URL: nodejs#26269
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 28, 2019

Landed in 4db10ed, 8bb3092 🎉

I suggest others to try out this new mode (e.g., use util.inspect.defaultOptions.compact = 2). I strongly believe it is a much nicer and more readable debug output than anything we had so far.

@BridgeAR BridgeAR closed this Feb 28, 2019
addaleax pushed a commit that referenced this pull request Mar 1, 2019
This overloads the `compact` option from `util.inspect()`. If it's
set to a number, it is going to align all most inner entries on the
same lign if they adhere to the following:

* The entries do not exceed the `breakLength` options value.
* The entry is one of the local most inner levels up the the one
  provided in `compact`.

PR-URL: #26269
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Mar 1, 2019
When using `util.inspect()` with `compact` mode set to a number, all
array entries exceeding 6 are going to be grouped together into
logical parts.

PR-URL: #26269
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR BridgeAR mentioned this pull request Mar 4, 2019
BridgeAR added a commit that referenced this pull request Mar 5, 2019
Notable Changes

* n-api:
  * Implement date object (Jarrod Connolly)
    (#25917)
* util:
  * Group array elements together (Ruben Bridgewater)
    (#26269)
  * Add compact depth mode (Ruben Bridgewater)
    (#26269)
* worker:
  * Improve integration with native addons (Anna Henningsen)
    (#26175)
BridgeAR added a commit that referenced this pull request Mar 5, 2019
Notable Changes

* n-api:
  * Implement date object (Jarrod Connolly)
    #25917
* util:
  * Add compact depth mode for `util.inspect()` (Ruben Bridgewater)
    #26269
* worker:
  * Improve integration with native addons (Anna Henningsen)
    #26175
  * MessagePort.prototype.onmessage takes arguments closer to the Web
    specification now (Anna Henningsen)
    #26082
Trott pushed a commit to Trott/io.js that referenced this pull request Mar 6, 2019
Notable Changes

* n-api:
  * Implement date object (Jarrod Connolly)
    nodejs#25917
* util:
  * Add compact depth mode for `util.inspect()` (Ruben Bridgewater)
    nodejs#26269
* worker:
  * Improve integration with native addons (Anna Henningsen)
    nodejs#26175
  * MessagePort.prototype.onmessage takes arguments closer to the Web
    specification now (Anna Henningsen)
    nodejs#26082
@BridgeAR BridgeAR deleted the improve-inspect-compact branch January 20, 2020 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

9 participants