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

Add indent option to util.inspect? #14638

Closed
gla5001 opened this issue Aug 5, 2017 · 15 comments
Closed

Add indent option to util.inspect? #14638

gla5001 opened this issue Aug 5, 2017 · 15 comments
Labels
discuss Issues opened for discussions and feedbacks. feature request Issues that request new features to be added to Node.js. util Issues and PRs related to the built-in util module.

Comments

@gla5001
Copy link
Contributor

gla5001 commented Aug 5, 2017

  • Version: master
  • Platform: all
  • Subsystem: util

It was mentioned in comment on PR, that it might useful to have 'real indenting' for util.inspect via a new option passed in. This is to make it function more like JSON.stringify with passing in a space specifier. Is this something that people would like to see? Would it be useful?

Refs: #14545

@mscdex mscdex added util Issues and PRs related to the built-in util module. feature request Issues that request new features to be added to Node.js. labels Aug 5, 2017
@tniessen tniessen added the discuss Issues opened for discussions and feedbacks. label Aug 5, 2017
@benjamingr
Copy link
Member

I'd like to get feedback from some of the heaviest users of util.inspect (mainly logging libraries) on this.

I'm personally +1.

@silverwind
Copy link
Contributor

silverwind commented Aug 5, 2017

One question is how the interaction of breakLength and the hypothetical indent should be. Should it still break off lines when indentation is enabled, possibly destroying the formatting?

@refack
Copy link
Contributor

refack commented Aug 5, 2017

how the interaction with breakLength and the hypothetical indent should be

🤔
I can't think of an obvious way it will interfere... seems to me that is we simply add \n after {/[ and before }/] it becomes similar to emca262 compliant.

> console.log(util.inspect(o, {breakLength:4}))
{ a: 1,
  b: 2,
  c: 3,
  d: 4,
  e:
   [ 1,
     2,
     3,
     4,
     { f: 6,
       g: [Array] } ] }

If anyone can find an example case IMHO it should also go into the current tests.

[refack: edited claim for spec compliance]

@silverwind
Copy link
Contributor

silverwind commented Aug 5, 2017

I can't think of an obvious way

Looks like I actually misinterpreted the breakLength description. There should actually be no issue, as indent should always "break" objects anyways effectively working like breakLength of 0.

@refack
Copy link
Contributor

refack commented Aug 7, 2017

ping @domenic, you had reservations to share?

@domenic
Copy link
Contributor

domenic commented Aug 8, 2017

I don't see the motivation for this. Despite claims above, this has nothing to do with spec compliance. This is just someone's subjective formatting preferences. The name "indent" is especially bad; the lines are already indented. This is actually about inserting newlines (again, at a place that one contributor thinks looks nicer).

I don't think util.format should be a general-purpose formatting with additional options for everyone's aesthetic preferences. I think that's better left to libraries. The existing formatter is a great balance of readability and conciseness.

@refack
Copy link
Contributor

refack commented Aug 8, 2017

@domenic I see your point. And I don't claim that this will make anything "up to spec" but the current implementation of the breakLength algorithm was also an arbitrary implementation - de15214#diff-38d4e5ad222633775fc89be4caa69a09R162, and I'd rather we give the users an option to indent in a way that is similar to JSON.stringify which is up to spec.

P.S. we're talking about util.inspect, not util.format, hence this should not affect console.log.

@gla5001
Copy link
Contributor Author

gla5001 commented Aug 8, 2017

agree that 'indent' is not the best name for the option if we choose to move forward with this. Maybe something like prettyFormat or something

@gla5001
Copy link
Contributor Author

gla5001 commented Aug 11, 2017

So this discussion has been pretty quiet for several days, how do I know if its worth working on or not?

@silverwind
Copy link
Contributor

I'd interpret the current responses as being not enough to warrant an implementation. Let's give it a bit more time and otherwise close it.

@TimothyGu
Copy link
Member

As others have articulated, util.inspect() is already an opinionated function. More "styles" of inspection belongs in a user module, not in util.inspect(). However, I do think an API to change the number of spaces used / the indentation character sounds like a fine idea, but implementation-wise it might not be as simple as one would think it is.

  1. Format. We currently have a somewhat odd (but concise) indentation scheme for arrays. Note how there's only one space before [:

    { a: 1,
      d: 4,
      e:
       [ 1,
         2,
         3,
         4,
         { f: 6,
           g: [Array] } ] }

    It would be impossible to replicate this with tab delimiter, and it wouldn't look as good with non-2-space indentations:

    {   a: 1,
        d: 4,
        e:
         [   1,
             2,
             3,
             4,
             {   f: 6,
                 g: [Array]   }   ]   }
    // lmao
  2. Custom inspection. Currently indentation is handled in the custom inspection function (if one is provided). E.g. the one for URLSearchParams:

    return `${this.constructor.name} {\n ${output.join(',\n ')} }`;

    which provides the following fairly attractive output:

    URLSearchParams {
      'key' => 'value',
      'key' => 'value',
      'key' => 'value',
      'key' => 'value',
      'key' => 'value',
      'key' => 'value',
      'key' => 'value',
      'key' => 'value',
      'key' => 'value',
      'key' => 'value' }

    It would introduce much churn if we were to support changing indentation as the styles wouldn't match with existing code written to conform to our current style.

So for these reasons, I'm leaning towards closing this ticket as well.

@refack
Copy link
Contributor

refack commented Aug 12, 2017

Can anyone see a path forward? We have a opinionated implementation (breakLength) but there is a more standardized format (JSON.stringify(obj, null, 2)).

  1. Should we try to deprecate breakLength?
  2. change it in a semver-major fashion?

BTW: AFAICT

node/lib/internal/url.js

Lines 189 to 191 in ab2b331

if (length > ctx.breakLength) {
return `${this.constructor.name} {\n ${output.join(',\n ')} }`;
} else if (output.length) {
is the only custom inspection that supports breakLength (https://github.com/nodejs/node/search?utf8=%E2%9C%93&q=breakLength&type=)

@TimothyGu
Copy link
Member

I'm closing this issue as there seems to be very limited support for this feature from Collaborators (myself included), and as the discussion has died down.

@cirosantilli
Copy link

cirosantilli commented Aug 7, 2020

This feature would be a huge improvement to the entire Node.js ecosystem. You've got to be able to view your objects from the terminal out of box without third party modules for debugging, this affects everyone. Non pretty inspect is just too unreadable.

If anything is greenlighted in that direction, I can try to implement.

@BridgeAR
Copy link
Member

BridgeAR commented Aug 7, 2020

It would be a lot easier to implement this by now than three years ago. We internally use nesting levels that could just be repeated by the named indentation (or we allow numbers).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. feature request Issues that request new features to be added to Node.js. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

No branches or pull requests

10 participants