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: truncate inspect array and typed array #6334

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 21, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

util

Description of change

As an alternative to #5070, set the max length of Arrays/TypedArrays in util.inspect() to 100 and provide a maxArrayLength option to override.

@jasnell jasnell added util Issues and PRs related to the built-in util module. semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 21, 2016
@jasnell
Copy link
Member Author

jasnell commented Apr 22, 2016

Refs: #4905

@jasnell
Copy link
Member Author

jasnell commented Apr 22, 2016

Refs: #3648

@@ -179,6 +179,10 @@ formatted string:
- `customInspect` - if `false`, then custom `inspect(depth, opts)` functions
defined on the objects being inspected won't be called. Defaults to `true`.

- `maxArrayLength` - specifies the maximum number of Array and TypedArray
elements to include when formatting. Defaults to `100`. Set to `Infinity`
to show all Array elements. Set to 0 or negative to show no array elements.
Copy link
Member

Choose a reason for hiding this comment

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

tiny nits: Maybe capitalize Array elements consistently in this line, and enclose the constant 0 in backticks?

@addaleax
Copy link
Member

Maybe copy the added tests to cover TypedArray, too? Either way, LGTM.

@jasnell
Copy link
Member Author

jasnell commented Apr 22, 2016

@addaleax ... done! ;-)

@silverwind
Copy link
Contributor

What's so special about arrays? Why not make it a more general maxBytes that would apply to objects and strings too? -1 from me in the current form.

@silverwind
Copy link
Contributor

Oh, and the option for infinity is inconsistent with the depth option of the same function, where null means unrestricted logging.

@jasnell
Copy link
Member Author

jasnell commented Apr 22, 2016

We already have depth for objects. Would not be opposed to adding a limit for strings. maxBytes could be a bit difficult to track based on how inspect is building things out. Not impossible, of course, just a bit more complicated. Perhaps we can make the option maxLength and have it mean maximum number of characters when inspecting a string, and mean maximum number of elements when inspecting an array?

@jasnell
Copy link
Member Author

jasnell commented Apr 29, 2016

oh, and to answer the question about what's special about arrays ... attempting to inspect an array that's too large brings the node process down. I have no issue with evolving this to place other limits, but limiting the array output is the bit we currently have open issues on. See #5070

@jasnell
Copy link
Member Author

jasnell commented Apr 29, 2016

Updated, PTAL

@addaleax
Copy link
Member

Still LGTM.

I’d say arrays are also special because they are the appropriate data type for (possibly large amounts) of homogeneous data, so omitting out a number of items is probably usually okay. Maybe something like this change would make sense for strings too, but not for objects, because there would be no natural way of guessing which keys to leave out when printing.

@jasnell
Copy link
Member Author

jasnell commented Apr 29, 2016

@silverwind ... any further thoughts on this? do you still object?

@silverwind
Copy link
Contributor

@jasnell no, seems reasonable. I'm only concerned about the option. depth takes null, maxArrayLength takes null or Infinity. How about making it just null for consistency?

@jasnell
Copy link
Member Author

jasnell commented Apr 30, 2016

@silverwind ... I can do that... but how about only in docs? If someone happens to pass Infinity it would still just work.

@mcollina
Copy link
Member

LGTM

@jasnell
Copy link
Member Author

jasnell commented Apr 30, 2016

@jasnell
Copy link
Member Author

jasnell commented Apr 30, 2016

@silverwind ... updated the doc to only mention setting maxArrayLength to null to specify no limit. Infinity can still be passed and it would work, but the only documented option is null to keep it consistent. Also made one other minor cleanup tweak. Running CI now. Assuming CI is green and there are no objections, I'll land on Monday.

@jasnell
Copy link
Member Author

jasnell commented May 1, 2016

CI is green.

@@ -179,6 +179,10 @@ formatted string:
- `customInspect` - if `false`, then custom `inspect(depth, opts)` functions
defined on the objects being inspected won't be called. Defaults to `true`.

- `maxArrayLength` - specifies the maximum number of Array and TypedArray
elements to include when formatting. Defaults to `100`. Set to `null` to
show all array elements. Set to `0` or negative to show no array elements.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just set to Infinity for all elements?

Copy link
Member Author

Choose a reason for hiding this comment

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

See prior discussion.

@Fishrock123
Copy link
Contributor

lgtm if nits are addressed

@Fishrock123
Copy link
Contributor

Fishrock123 commented May 2, 2016

If this is already major, maybe prefer moving the preference to Infinity over null for the other option? Or would it be better to do that in a new PR?

@silverwind
Copy link
Contributor

Infinity also work on depth, so I guess it's fine to leave it undocumented:

> util.inspect({a:{a:{a:{a:1}}}})
'{ a: { a: { a: [Object] } } }'
> util.inspect({a:{a:{a:{a:1}}}}, {depth: null})
'{ a: { a: { a: { a: 1 } } } }'
> util.inspect({a:{a:{a:{a:1}}}}, {depth: Infinity})
'{ a: { a: { a: { a: 1 } } } }'

LGTM

@jasnell
Copy link
Member Author

jasnell commented May 2, 2016

@silverwind @addaleax @mcollina ... any thoughts on whether this has to be semver-major or do you think we can treat it as a bug fix?

@Fishrock123
Copy link
Contributor

Fishrock123 commented May 2, 2016

I don't think very many changes to util.inspect() should be considered major. It's primary purpose is creating human-readable output.

@silverwind
Copy link
Contributor

I guess semver-minor would be fine.

@addaleax
Copy link
Member

addaleax commented May 2, 2016

+1 to semver-minor.

@jasnell jasnell added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels May 2, 2016
@jasnell
Copy link
Member Author

jasnell commented May 3, 2016

New CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/2484/
will land after if it's green.

As an alternative to nodejs#5070,
set the max length of Arrays/TypedArrays in util.inspect() to
`100` and provide a `maxArrayLength` option to override.
@jasnell
Copy link
Member Author

jasnell commented May 3, 2016

Green. unrelated flaky timeout in rpi.

jasnell added a commit that referenced this pull request May 3, 2016
As an alternative to #5070,
set the max length of Arrays/TypedArrays in util.inspect() to
`100` and provide a `maxArrayLength` option to override.

PR-URL: #6334
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@jasnell
Copy link
Member Author

jasnell commented May 3, 2016

Landed in a2e5719

@mcollina
Copy link
Member

mcollina commented May 3, 2016

Ok for semver-minor.

@jasnell
Copy link
Member Author

jasnell commented May 3, 2016

Thank you @mcollina :-)

Fishrock123 pushed a commit that referenced this pull request May 4, 2016
As an alternative to #5070,
set the max length of Arrays/TypedArrays in util.inspect() to
`100` and provide a `maxArrayLength` option to override.

PR-URL: #6334
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request May 4, 2016
As an alternative to nodejs#5070,
set the max length of Arrays/TypedArrays in util.inspect() to
`100` and provide a `maxArrayLength` option to override.

PR-URL: nodejs#6334
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Fishrock123 added a commit that referenced this pull request May 5, 2016
* assert: `deep{Strict}Equal()` now works correctly with circular
references. (Rich Trott) #6432
* debugger: Arrays are now formatted correctly in the debugger repl.
(cjihrig) #6448
* deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu)
#6550
* net: Introduced a `Socket#connecting` property. (Fedor Indutny)
#6404
  - Previously this information was only available as the undocumented,
internal `_connecting` property.
* process: Introduced `process.cpuUsage()`. (Patrick Mueller)
#6157
* stream: `Writable#setDefaultEncoding()` now returns `this`.
(Alexander Makarenko) #5040
* util: Two new additions to `util.inspect()`:
  - Added a `maxArrayLength` option to truncate the formatting of
Arrays. (James M Snell) #6334
    - This is set to `100` by default.
  - Added a `showProxy` option for formatting proxy intercepting
handlers. (James M Snell) #6465
    - Inspecting proxies is non-trivial and as such this is off by
default.

PR-URL: #6557
Fishrock123 added a commit that referenced this pull request May 5, 2016
* assert: `deep{Strict}Equal()` now works correctly with circular
references. (Rich Trott) #6432
* debugger: Arrays are now formatted correctly in the debugger repl.
(cjihrig) #6448
* deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu)
#6550
  - Please see our blog post for more info on the security contents of this release:
  - https://nodejs.org/en/blog/vulnerability/openssl-may-2016/
* net: Introduced a `Socket#connecting` property. (Fedor Indutny)
#6404
  - Previously this information was only available as the undocumented,
internal `_connecting` property.
* process: Introduced `process.cpuUsage()`. (Patrick Mueller)
#6157
* stream: `Writable#setDefaultEncoding()` now returns `this`.
(Alexander Makarenko) #5040
* util: Two new additions to `util.inspect()`:
  - Added a `maxArrayLength` option to truncate the formatting of
Arrays. (James M Snell) #6334
    - This is set to `100` by default.
  - Added a `showProxy` option for formatting proxy intercepting
handlers. (James M Snell) #6465
    - Inspecting proxies is non-trivial and as such this is off by
default.

PR-URL: #6557
Fishrock123 added a commit that referenced this pull request May 5, 2016
* assert: `deep{Strict}Equal()` now works correctly with circular
references. (Rich Trott) #6432
* debugger: Arrays are now formatted correctly in the debugger repl.
(cjihrig) #6448
* deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu)
#6550
- Please see our blog post for more info on the security contents of
this release:
- https://nodejs.org/en/blog/vulnerability/openssl-may-2016/
* net: Introduced a `Socket#connecting` property. (Fedor Indutny)
#6404
- Previously this information was only available as the undocumented,
internal `_connecting` property.
* process: Introduced `process.cpuUsage()`. (Patrick Mueller)
#6157
* stream: `Writable#setDefaultEncoding()` now returns `this`.
(Alexander Makarenko) #5040
* util: Two new additions to `util.inspect()`:
- Added a `maxArrayLength` option to truncate the formatting of
Arrays. (James M Snell) #6334
- This is set to `100` by default.
- Added a `showProxy` option for formatting proxy intercepting
handlers. (James M Snell) #6465
- Inspecting proxies is non-trivial and as such this is off by
default.

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

5 participants