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.

Sign up for GitHub

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

Jump to bottom

util: improve inspect #20802

Closed
wants to merge 4 commits into from
Closed

util: improve inspect #20802

wants to merge 4 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented May 17, 2018 •
edited
Loading

Please check the commit messages for the details.

Refs: #20253

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 May 17, 2018
addaleax
addaleax approved these changes May 17, 2018
View reviewed changes
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.

LGTM as semver-major

@addaleax addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 17, 2018
@BridgeAR
Copy link
Member Author

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 17, 2018
@BridgeAR BridgeAR requested a review from a team May 17, 2018 15:02
mcollina
mcollina approved these changes May 17, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member

Trott commented May 19, 2018

Would be curious to see CITGM results as I could see this impacting test suites. (But maybe not?)

@BridgeAR
Copy link
Member Author

@addaleax about this being semver-major: we actually have the following in our docs:

The output of util.inspect may change at any time and should not be depended upon programmatically.

So I personally do not think this is semver-major. When added, it was somewhat meant for changes like these as far as I can tell.

@addaleax
Copy link
Member

@BridgeAR Yes, this is definitely in the “erring on the side of caution” category. I know that people shouldn’t depend on it, but I’m not sure this change is trivial enough to have a reasonable expectation that it doesn’t break people’s code.

If there is a good reason for this not being semver-major (as in, a significant advantage for us), we can remove the label.

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1418/

@BridgeAR
util: wrap error in brackets without stack
13a3ae3 
This aligns the visualization of an error with no stack traces set
to zero just as it is done in case the error has no stack trace.
@BridgeAR
util: remove erroneous whitespace
5639463 
When inspecting nested objects some times a whitespace was added at
the end of a line. This fixes this erroneous space.

Besides that the `breakLength` was not followed if a single property
was longer than the breakLength. It will now break a single property
into the key and value in such cases.
@BridgeAR
util: fix inspected stack indentation
96b303c 
Error stacks and multiline error messages were not correct indented.
This is fixed by this patch.
@BridgeAR
util: improve error inspection
9c1d559 
When inspecting errors with extra properties while setting the
compact option to false, it will now return:

[Error: foo] {
    at repl:1:5
    at Script.runInThisContext (vm.js:89:20)
  bla: true
}

Instead of:

Error: foo
    at repl:1:5
    at Script.runInThisContext (vm.js:91:20) {
  bla: true
}
@BridgeAR
Copy link
Member Author

Rebased due to conflicts.

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

Since the conflicts required some more work, PTAL.

@addaleax one main benefit would be easier backports.

@BridgeAR
Copy link
Member Author

As a note: nothing seem to show up on CITGM.

@mcollina
Copy link
Member

Still LGTM

BridgeAR added a commit to BridgeAR/node that referenced this pull request May 21, 2018
@BridgeAR
util: wrap error in brackets without stack
afd290d 
This aligns the visualization of an error with no stack traces set
to zero just as it is done in case the error has no stack trace.

PR-URL: nodejs#20802
Refs: nodejs#20253
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request May 21, 2018
@BridgeAR
util: remove erroneous whitespace
8de8372 
When inspecting nested objects some times a whitespace was added at
the end of a line. This fixes this erroneous space.

Besides that the `breakLength` was not followed if a single property
was longer than the breakLength. It will now break a single property
into the key and value in such cases.

PR-URL: nodejs#20802
Refs: nodejs#20253
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request May 21, 2018
@BridgeAR
util: fix inspected stack indentation
e852289 
Error stacks and multiline error messages were not correct indented.
This is fixed by this patch.

PR-URL: nodejs#20802
Refs: nodejs#20253
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request May 21, 2018
@BridgeAR
util: improve error inspection
c041a2e 
When inspecting errors with extra properties while setting the
compact option to false, it will now return:

[Error: foo] {
    at repl:1:5
    at Script.runInThisContext (vm.js:89:20)
  bla: true
}

Instead of:

Error: foo
    at repl:1:5
    at Script.runInThisContext (vm.js:91:20) {
  bla: true
}

PR-URL: nodejs#20802
Refs: nodejs#20253
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BridgeAR
Copy link
Member Author

Thanks.

Landed in 7f0f978...c041a2e 🎉

@BridgeAR BridgeAR closed this May 21, 2018
@BridgeAR
Copy link
Member Author

@addaleax I still think it would be best not to have this as semver-major.

@addaleax
Copy link
Member

@BridgeAR If you feel strongly about this, you can remove the label… I don’t think this is something we’d want to backport to an LTS line, though

@addaleax addaleax mentioned this pull request May 22, 2018
Merged
@BridgeAR BridgeAR deleted the improve-error-inspection branch January 20, 2020 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@BridgeAR @Trott @addaleax @mcollina @jasnell @trivikr @nodejs-github-bot