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

console: add `inspectOptions` option #24978

Closed
wants to merge 3 commits into
base: master
from

Conversation

@BridgeAR
Copy link
Member

BridgeAR commented Dec 12, 2018

Add an inspectOptions option to the console constructor. That
way it's possible to define all inspection defaults for each
console instance instead of relying on the inspect() defaults.

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
console: add `inspectOptions` option
Add an `inspectOptions` option to the `console` constructor. That
way it's possible to define all inspection defaults for each
`console` instance instead of relying on the `inspect()` defaults.

@BridgeAR BridgeAR requested a review from addaleax Dec 12, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 12, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 12, 2018

Show resolved Hide resolved doc/api/console.md Outdated
fixup: fix option name
Co-Authored-By: BridgeAR <ruben@bridgewater.de>
@BridgeAR

This comment has been minimized.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 14, 2018

This could use some reviews.

@BridgeAR BridgeAR requested review from devsnek , jasnell , mcollina and ryzokuken Dec 14, 2018

@mcollina
Copy link
Member

mcollina left a comment

How would a user change the inspectOptions of the global/default console? It seems a needed feature.

If that's possible, we should document it and maybe add a test for it.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 14, 2018

@mcollina that's possible by changing util.inspect.defaultOptions. It is document in the util part. However, I would not want to document this any more prominent, since it's already used in userland modules and that's a pretty bad anti pattern. I am afraid more modules might change these defaults instead of relying on an individual logging instance (where it's totally fine to change the settings to their own needs).

The question does not seem to be directly related to this PR though.

@mcollina
Copy link
Member

mcollina left a comment

LGTM

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 15, 2018

Show resolved Hide resolved doc/api/errors.md Outdated
Show resolved Hide resolved lib/internal/console/constructor.js
@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 16, 2018

@Trott Trott removed the author ready label Dec 16, 2018

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 16, 2018

Removed author ready pending result of the conversation started in #24978 (comment). Just being cautious. No objections from me to landing.

has enough approvals

BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 17, 2018

console: add `inspectOptions` option
Add an `inspectOptions` option to the `console` constructor. That
way it's possible to define all inspection defaults for each
`console` instance instead of relying on the `inspect()` defaults.

PR-URL: nodejs#24978
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 17, 2018

Landed in be3ae33

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Dec 25, 2018

The tests for this PR fail on 11.x. Would someone be willing to open a backport?

@targos targos added this to Backport requested in v11.x Dec 28, 2018

@targos targos referenced this pull request Jan 1, 2019

Merged

console: improve inspectOptions validation #25090

3 of 3 tasks complete
@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Jan 9, 2019

This relies on other console PRs and we should wait till those are backported. This should land cleanly afterwards.

BridgeAR added a commit that referenced this pull request Jan 10, 2019

console: add `inspectOptions` option
Add an `inspectOptions` option to the `console` constructor. That
way it's possible to define all inspection defaults for each
`console` instance instead of relying on the `inspect()` defaults.

PR-URL: #24978
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Jan 10, 2019

I was able to backport this directly to the staging branch due to recent console backports.

@BridgeAR BridgeAR moved this from Backport requested to Backported in v11.x Jan 10, 2019

addaleax added a commit that referenced this pull request Jan 14, 2019

console: add `inspectOptions` option
Add an `inspectOptions` option to the `console` constructor. That
way it's possible to define all inspection defaults for each
`console` instance instead of relying on the `inspect()` defaults.

PR-URL: #24978
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

console: add `inspectOptions` option
Add an `inspectOptions` option to the `console` constructor. That
way it's possible to define all inspection defaults for each
`console` instance instead of relying on the `inspect()` defaults.

PR-URL: nodejs#24978
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>

@BridgeAR BridgeAR referenced this pull request Jan 16, 2019

Merged

v11.7.0 proposal #25537

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019

2019-01-17, Version 11.7.0 (Current), @BridgeAR
Notable Changes

* compression / zlib:
  * Added brotli support (Anna Henningsen and Zach Vacura)
    nodejs#24938
* console:
  * Added `inspectOptions` option (Ruben Bridgewater)
    nodejs#24978
* crypto:
  * Always accept private keys as public keys (Tobias Nießen)
    nodejs#25217
* deps:
  * Upgrade npm to v6.5.0 (Jordan Harband)
    nodejs#25234
* fs:
  * Use internalBinding('fs') internally instead of
    process.binding('fs') (Masashi Hirano)
    nodejs#22478
* http(s):
  * Support overriding http\\s.globalAgent (Roy Sommer)
    nodejs#25170
* util:
  * Inspect ArrayBuffers contents closely (Ruben Bridgewater)
    nodejs#25006

PR-URL: nodejs#25537

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019

2019-01-17, Version 11.7.0 (Current), @BridgeAR
Notable Changes

* compression / zlib:
  * Added brotli support (Anna Henningsen and Zach Vacura)
    nodejs#24938
* console:
  * Added `inspectOptions` option (Ruben Bridgewater)
    nodejs#24978
* crypto:
  * Always accept private keys as public keys (Tobias Nießen)
    nodejs#25217
* deps:
  * Upgrade npm to v6.5.0 (Jordan Harband)
    nodejs#25234
* fs:
  * Use internalBinding('fs') internally instead of
    process.binding('fs') (Masashi Hirano)
    nodejs#22478
* http(s):
  * Support overriding http\\s.globalAgent (Roy Sommer)
    nodejs#25170
* util:
  * Inspect ArrayBuffers contents closely (Ruben Bridgewater)
    nodejs#25006
* worker:
  * Expose workers by default and remove `--experimental-worker` flag
    (Anna Henningsen) nodejs#25361

PR-URL: nodejs#25537

BridgeAR added a commit that referenced this pull request Jan 18, 2019

2019-01-17, Version 11.7.0 (Current), @BridgeAR
Notable Changes

* compression / zlib:
  * Added brotli support (Anna Henningsen and Zach Vacura)
    #24938
* console:
  * Added `inspectOptions` option (Ruben Bridgewater)
    #24978
* crypto:
  * Always accept private keys as public keys (Tobias Nießen)
    #25217
* deps:
  * Upgrade npm to v6.5.0 (Jordan Harband)
    #25234
* fs:
  * Use internalBinding('fs') internally instead of
    process.binding('fs') (Masashi Hirano)
    #22478
* http(s):
  * Support overriding http\\s.globalAgent (Roy Sommer)
    #25170
* util:
  * Inspect ArrayBuffers contents closely (Ruben Bridgewater)
    #25006
* worker:
  * Expose workers by default and remove `--experimental-worker` flag
    (Anna Henningsen) #25361

PR-URL: #25537

@MylesBorins MylesBorins referenced this pull request Jan 24, 2019

Merged

v11.8.0 proposal #25687

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment