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.inspect cuts off strings too early #27690

Closed
devsnek opened this issue May 14, 2019 · 29 comments
Closed

util.inspect cuts off strings too early #27690

devsnek opened this issue May 14, 2019 · 29 comments
Labels
console Issues and PRs related to the console subsystem. util Issues and PRs related to the built-in util module.

Comments

@devsnek
Copy link
Member

devsnek commented May 14, 2019

  • Version: 12.2.0
  • Subsystem: util/console

strings are cut off well below the limit of the tty's capacity, resulting in difficult to grok output

@BridgeAR

@devsnek devsnek added util Issues and PRs related to the built-in util module. console Issues and PRs related to the console subsystem. labels May 14, 2019
@BridgeAR
Copy link
Member

@devsnek this relies upon the breakLength option which is currently set to 80 characters (it was actually at 60 before v12). I would like to set the default in relation to the terminal columns but util.inspect is used in lots of contexts, so always relying upon that might cause trouble. What we could do is to have a console specific default that checks the column size similar to our color handling in console.

@BridgeAR
Copy link
Member

We can do something similar in the REPL.

@targos
Copy link
Member

targos commented May 14, 2019

Having a special behavior for console or REPL output sound good to me

@silverwind
Copy link
Contributor

Why not set it to Infinityon the REPL? All terminals wrap long lines and not breaking it ourself allows the terminal to re-wrap the output on terminal resize (only some terminals do that, though).

@BridgeAR
Copy link
Member

@silverwind the output is split in a relatively natural way right now and it will improve readability for long strings, especially when the screen has a high resolution. The terminal wrapping is difficult to read and follow and has no knowledge about the actual output, so I don't think that's a good idea. I don't think that resizing the terminal is going to be a big issue. Especially if we keep the breakLength below a certain threshold.

Switching to a dynamic range between e.g., 80-120 (maybe up to 150, but lines of that length often already become harder to follow) would already improve the situation.
Most linters are configured for a maximum line length of 80 or 100 characters and that's because it's just easier to follow lines of that length.

@silverwind
Copy link
Contributor

I'd agree to something like Math.min(120, tty.columns). Thought if you want to go for tty.columns, you may as well set it to Infinity because it has the same effect (until the terminal is resized, then it will get ugly).

@devsnek
Copy link
Member Author

devsnek commented May 16, 2019

actually can this feature just be disabled by default/in console? i'd rather my terminal wrapped long items than node manually splitting the data. it makes it much harder to copy/paste and things of that nature.

@BridgeAR
Copy link
Member

@devsnek it is possible to opt out by resetting the inspection default (e.g., util.inspect.defaultOptions.compact = true).

@devsnek
Copy link
Member Author

devsnek commented May 16, 2019

yeah, but I think the new default is less readable and less usable. we didn't get any complaints before, and now we've already had two issues on it.

@BridgeAR
Copy link
Member

BridgeAR commented May 16, 2019

@devsnek I think it is a bit early to jump to conclusions :) and I wouldn't consider asking for possibilities to switch as complaining.

I am happy to implement what we discussed above and I hope it'll resolve your main concern.

Other than that it's always possible to opt into using the old mode as described above (and there are multiple ways to set these settings depending on what exactly you do).

@silverwind

Thought if you want to go for tty.columns, you may as well set it to Infinity because it has the same effect (until the terminal is resized, then it will get ugly).

I would not use tty.columns directly. Instead, I would like to use a proportion (likely not linear).

@devsnek
Copy link
Member Author

devsnek commented May 22, 2019

just another example:

"does this start with the wasm magic number" was my goal. i think most people's eyes will be immediately drawn down the first column, not across the first row. this seems like an insanely confusing way to display an array.

@cjihrig
Copy link
Contributor

cjihrig commented May 26, 2019

Linking to #27915 so that related issues are tracked from here.

@cjihrig
Copy link
Contributor

cjihrig commented May 27, 2019

@devsnek if you or anyone else wants to open a PR changing the util.inspect.defaultOptions.compact default to true, I'd sign off on it. There has been a very large amount of churn in util.inspect(), and that seems to be the simplest way to deal with the issues we're encountering, without adding yet another util.inspect() option.

@BridgeAR
Copy link
Member

Just as a heads up: I am currently looking into improving the situation in general and I'll gather some feedback during the summit.

BridgeAR added a commit to BridgeAR/node that referenced this issue Jun 4, 2019
Using the `util.inspect` `compact` mode set to something else than
`true` resulted in breaking long lines in case the line would exceed
the `breakLength` option and if it contained whitespace and or new
lines.

It turned out that this behavior was less useful than originally
expected and it is now changed to only break on line breaks if the
`breakLength` option is exceeded for the inspected string. This should
be align better with the user expectation than the former behavior.

Fixes: nodejs#27690
@BridgeAR
Copy link
Member

BridgeAR commented Jun 4, 2019

After talking to a couple of people at the summit I think it's best to change the long line behavior. I opened #28055 to address this issue by only splitting lines above the breakLength in case the string contains line breaks and it will only split the lines at exactly those locations.

The feedback I received was also to keep the array grouping mostly as it is: the order should not be changed, since it's otherwise not possible to copy the array into the REPL or other places anymore. We currently align all entries at the end of the string and it's likely better to align them at the start for all arrays that contain other types than numbers. I'll also open another PR to address that soon.

The issue about console.table was a regression that is already fixed, so that does not seem to be an issue anymore.

This should hopefully address all concerns about the current output.

@devsnek
Copy link
Member Author

devsnek commented Jun 4, 2019

@BridgeAR I was saying to change the grouping not the order.

[
a, b, c,
d, e, f
]

vs

[
a,     b,
c,     d,
e,     f,
]

I think the first one is a lot more readable

@BridgeAR
Copy link
Member

BridgeAR commented Jun 4, 2019

@devsnek right now the algorithm is going to try to create an ideal square with an best effort approach. It also takes the breakLength into account.

The input length often varies in size and if that's the case I use the longest entry as "spacer". In your example above the string '... 28 more items' is significantly longer than all other entries and it is therefore more space in-between the entries. I could add a special handling for the "extra entries" part but that increases the complexity of the algorithm. I guess that is what you would like to have?

@devsnek
Copy link
Member Author

devsnek commented Jun 4, 2019

@BridgeAR

[
  a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, q, r, s, t,
  u, v, w, x, y, z, aa, bb, cc, dd, ee, ff, gg, hh, ii, jj,
  ... 1337 more items
]

since we're producing output meant to be read left to right we should group it into rows not columns. imo we should also aim to keep newlines to a minimum to aid copy-pasting and take advantage of terms that can resize.

@BridgeAR
Copy link
Member

BridgeAR commented Jun 4, 2019

@devsnek I'll open a PR to special handle maxArrayLength. That will automatically result in more condensed grouping. I still think we should keep the column based approach, since it's otherwise difficult to see where things start or stop, depending on the input.

@devsnek
Copy link
Member Author

devsnek commented Jun 4, 2019

i don't know what maxArrayLength or "where things start or stop" mean

@targos
Copy link
Member

targos commented Jun 4, 2019

@BridgeAR the main issue I see is not about maxArrayLength or column-based alignment.

Take this example:

$ node -p "Array(200).fill(0)"
[
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
                   0,                  0,                  0,                  0,
  ... 100 more items
]

All numbers take the same space, so why aren't there more of them on each row?

@BridgeAR
Copy link
Member

BridgeAR commented Jun 4, 2019

I just opened a PR to address this. @devsnek @targos PTAL at #28059

@BridgeAR
Copy link
Member

BridgeAR commented Jun 4, 2019

I'll open another PR on top of the already opened ones that will reduce the space between columns in case the space for the whole column is more than it has to be. I also plan on auto detecting the types of the entries of the array and and only print arrays with only numbers ordered to the right. All other arrays would be ordered to the left.

A screenshot of what I'm talking about:

image

@devsnek
Copy link
Member Author

devsnek commented Jun 4, 2019

still.. given computers generally have more width than height, it feels like this wastes a lot of screen space and doesn't read as naturally.

@devsnek
Copy link
Member Author

devsnek commented Jun 4, 2019

@BridgeAR also as another example i came upon while making a comment for your pr... the output here initially confused me and, once i understood what it was, prevented me from copying it, making me have to use console.log(_)

@BridgeAR
Copy link
Member

BridgeAR commented Jun 4, 2019

I have a PR that increases the bias towards more columns and also increases the maximum columns in general. I think it's best to keep the square as rule of thumb and not increase the columns above my upcoming PR, even though I now move the columns together in case it fits best that way:

image

image

The grouping is significantly better readability wise than anything we had before and my last improvements will hopefully iron out the edges. I suggest to keep it with those improvements for a while and get some further feedback. Users will still be able to change the defaults if they want (e.g., increase the breakLength which would result in more columns).

@devsnek your last example will work as you expect with #28055 applied.

@silverwind
Copy link
Contributor

silverwind commented Jun 7, 2019

By the way: Is anyone else bothered by the fact that we emit single quotes on strings? Would love those to be double quotes so it would represent valid JSON (to some extend) 😉

antsmartian pushed a commit that referenced this issue Jun 9, 2019
This makes sure that large arrays with lots of small entries ignore
the `... n more item(s)` part since it often resulted in output that
users did not expect.

Now that part is printed on a separate line to indicate extra entries.

PR-URL: #28059
Refs: #27690
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@BridgeAR
Copy link
Member

@silverwind I personally like single quotes a tiny bit more while I do not have a strong opinion on it. Since this is pretty off-topic to this specific issue, would you be so kind and open a new issue for that?

@devsnek this issue should be resolved due to #28055 and #28059. There is one more PR (#28070) to change some details about array grouping further (more columns and printing output more compact at times).
If this does not address all concerns, please just reopen the issue with further details.

BridgeAR added a commit to BridgeAR/node that referenced this issue Jun 17, 2019
This improves a couple minor things:

* Arrays that contain entries other than `number` or `bigint` are
  ordered to the left instead of the right.
* The bias towards more columns got increased. That mainly increases
  the number of columns for arrays that contain lots of short entries.
* Columns are now more dense in case they would otherwise have extra
  whitespace in-between two columns.
* The maximum columns got increased from 10 to 15.
* The maximum number of columns per `compact` was increased from
  3 to 4.

PR-URL: nodejs#28070
Refs: nodejs#27690
Reviewed-By: James M Snell <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this issue Jun 17, 2019
This makes sure that strongly deviating entry length are taken into
account while grouping arrays.

PR-URL: nodejs#28070
Refs: nodejs#27690
Reviewed-By: James M Snell <[email protected]>
BridgeAR added a commit that referenced this issue Jun 17, 2019
This makes sure that large arrays with lots of small entries ignore
the `... n more item(s)` part since it often resulted in output that
users did not expect.

Now that part is printed on a separate line to indicate extra entries.

PR-URL: #28059
Refs: #27690
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR added a commit that referenced this issue Jun 17, 2019
Using the `util.inspect` `compact` mode set to something else than
`true` resulted in breaking long lines in case the line would exceed
the `breakLength` option and if it contained whitespace and or new
lines.

It turned out that this behavior was less useful than originally
expected and it is now changed to only break on line breaks if the
`breakLength` option is exceeded for the inspected string. This should
be align better with the user expectation than the former behavior.

PR-URL: #28055
Fixes: #27690
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR added a commit that referenced this issue Jun 17, 2019
This improves a couple minor things:

* Arrays that contain entries other than `number` or `bigint` are
  ordered to the left instead of the right.
* The bias towards more columns got increased. That mainly increases
  the number of columns for arrays that contain lots of short entries.
* Columns are now more dense in case they would otherwise have extra
  whitespace in-between two columns.
* The maximum columns got increased from 10 to 15.
* The maximum number of columns per `compact` was increased from
  3 to 4.

PR-URL: #28070
Refs: #27690
Reviewed-By: James M Snell <[email protected]>
BridgeAR added a commit that referenced this issue Jun 17, 2019
This makes sure that strongly deviating entry length are taken into
account while grouping arrays.

PR-URL: #28070
Refs: #27690
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Jun 18, 2019
This improves a couple minor things:

* Arrays that contain entries other than `number` or `bigint` are
  ordered to the left instead of the right.
* The bias towards more columns got increased. That mainly increases
  the number of columns for arrays that contain lots of short entries.
* Columns are now more dense in case they would otherwise have extra
  whitespace in-between two columns.
* The maximum columns got increased from 10 to 15.
* The maximum number of columns per `compact` was increased from
  3 to 4.

PR-URL: #28070
Refs: #27690
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Jun 18, 2019
This makes sure that strongly deviating entry length are taken into
account while grouping arrays.

PR-URL: #28070
Refs: #27690
Reviewed-By: James M Snell <[email protected]>
@LukasBombach
Copy link

If you came here searching for a way to show the hidden items in you array, you got to pass maxArrayLength: Infinity

console.log(util.inspect(value, { maxArrayLength: Infinity }));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants