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

console: .table fall back to logging for function too #20681

Closed
wants to merge 1 commit into from

Conversation

ohbarye
Copy link
Contributor

@ohbarye ohbarye commented May 11, 2018

Hello @devsnek
I prepared a pull request for #20679.


Overview

According to the console.table document, it reads that it "falls back to just logging the argument if it can’t be parsed as tabular."

But it doesn't fall back when I give a function as its first argument. It logs an empty table.

Fixes: #20679

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • [ ] documentation is changed or added (I think it's not necessary this time)
  • commit message follows commit guidelines

According to the [console.table
document](https://github.com/nodejs/node/blob/master/doc/api/console.md#consoletabletabulardata-properties),
it reads that it "falls back to just logging the argument if it can’t be
parsed as tabular."

But it doesn't fall back when I give a function as its first argument.
It logs an empty table.

Fixes: nodejs#20679
@nodejs-github-bot nodejs-github-bot added the console Issues and PRs related to the console subsystem. label May 11, 2018
@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 11, 2018
@Trott
Copy link
Member

Trott commented May 11, 2018

This will break current behavior in the situation where a function has properties attached to it:

$ ./node
> foo = function () {}
[Function: foo]
> foo.bar = 'hey'
'hey'
> console.table(foo)
┌─────────┬────────┐
│ (index) │ Values │
├─────────┼────────┤
│   bar   │ 'hey'  │
└─────────┴────────┘
undefined
>

Honest question: Do we care?

In both situations, Chrome appears to publish a blank line. Firefox, in contrast, does what this PR does.

FWIW, my interpretation of the spec is that our current behavior is correct.

@devsnek
Copy link
Member

devsnek commented May 11, 2018

i'm fine with this change. if you want to get really into it you could check the getOwnPropertyNames and filter out 'length', 'name', 'arguments', 'caller', 'prototype' and if it still has anything left then display it as tabular data. (but i don't think thats a needed feature)

@Trott
Copy link
Member

Trott commented May 11, 2018

Thinking more about this, yeah, I don't object to this change. It's not my preference, but I can certainly see it this way, and we can always revisit if the spec ever gets more specific about how to handle functions.

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 18, 2018
According to the console.table documentation, it reads that it "falls
back to just logging the argument if it can’t be parsed as tabular."

But it doesn't fall back when I give a function as its first argument.
It logs an empty table. This is fixes by this commit.

PR-URL: nodejs#20681
Fixes: nodejs#20679
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BridgeAR
Copy link
Member

Landed in 0c852a1

@ohbarye congratulations on your first commit to Node.js! 🎉 You are now officially a contributor :-)

@BridgeAR BridgeAR closed this May 18, 2018
@ohbarye
Copy link
Contributor Author

ohbarye commented May 18, 2018

Thank you for your review and the warm welcome! :)

@ohbarye ohbarye deleted the fall-back-to-log-for-function branch May 18, 2018 16:32
MylesBorins pushed a commit that referenced this pull request May 22, 2018
According to the console.table documentation, it reads that it "falls
back to just logging the argument if it can’t be parsed as tabular."

But it doesn't fall back when I give a function as its first argument.
It logs an empty table. This is fixes by this commit.

PR-URL: #20681
Fixes: #20679
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@addaleax addaleax mentioned this pull request May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. console Issues and PRs related to the console subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

console: console.table doesn't fall back to just logging when given a function
8 participants