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

Update console.table docs. #20318

Closed
wants to merge 2 commits into from
Closed

Update console.table docs. #20318

wants to merge 2 commits into from

Conversation

sant123
Copy link

@sant123 sant123 commented Apr 26, 2018

Checklist
  • documentation is changed or added

@nodejs-github-bot nodejs-github-bot added console Issues and PRs related to the console subsystem. doc Issues and PRs related to the documentations. labels Apr 26, 2018
sant123 referenced this pull request Apr 26, 2018
PR-URL: #18137
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@Trott
Copy link
Member

Trott commented Apr 26, 2018

/ping @BridgeAR

@Trott
Copy link
Member

Trott commented Apr 26, 2018

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

Trott commented Apr 26, 2018

Commit message doesn't conform to the guidelines/conventions used, but especially for such a focused change, whoever is landing the code can certainly fix it.

In case that person is not me and whoever it is doesn't want to think about it, here's a suggested commit log:

doc: fix example in console.md

Update example code in console.md to be the correct code for generating the
output indicated.

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 26, 2018
@Trott
Copy link
Member

Trott commented Apr 26, 2018

Leave a 👍 here if you are a Collaborator in favor of fast-tracking this.

@vsemozhetbyt
Copy link
Contributor

Should an example of a call without properties be added?

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 26, 2018

Also not sure if this is a typo: ...and logit. and if it can be addressed here on landing, feel free to ignore)

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

LGTM with ignorable nits in comments)

@Trott
Copy link
Member

Trott commented Apr 26, 2018

Should an example of a call without properties be added?

@vsemozhetbyt Yes (although it doesn't have to be in this pull request if @sant123 just wants to get a quick correction in and not spend time perfecting the console.table() example code beyond fixing a clear mistake). If you'd like, maybe push a second commit to this branch yourself adding that example?

@vsemozhetbyt
Copy link
Contributor

@Trott If I push my own commit, will not this mess up with First-time contributor to Contributor promotion?

@Trott
Copy link
Member

Trott commented Apr 26, 2018

@Trott If I push my own commit, will not this mess up with First-time contributor to Contributor promotion?

If I had to guess, I wouldn't think so, but if you believe it might, you very well could be right!

@@ -367,7 +367,7 @@ console.table(undefined);
```

```js
console.table([{ a: 1, b: 'Y' }, { a: 'Z', b: 2 }]);
console.table([{ a: 1, b: 'Y' }, { a: 'Z', b: 2 }], ['a', 'b']);
Copy link
Member

Choose a reason for hiding this comment

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

The parameter properties is optional here.
The output is same even if ['a', 'b'] is not passed as properties

Copy link
Contributor

Choose a reason for hiding this comment

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

For me:

> console.table([{ a: 1, b: 'Y' }, { a: 'Z', b: 2 }]);
┌─────────┬──────────────────┐
 (index)       Values      
├─────────┼──────────────────┤
    0     { a: 1, b: 'Y' } 
    1     { a: 'Z', b: 2 } 
└─────────┴──────────────────┘

Copy link
Contributor

Choose a reason for hiding this comment

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

They are optional, but not with the same output it seems.

Copy link
Member

Choose a reason for hiding this comment

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

@vsemozhetbyt You're right. I just verified it with Node.js v10.0.0
My bad, earlier I'd verified it on Google Chrome v66 where the output was same even if ['a', 'b'] is not passed as properties

@vsemozhetbyt
Copy link
Contributor

I've pushed under the OP username and email, this can be reverted if I am wrong.

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/601/

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

we should fix the bug not document the bugged behaviour

@Trott
Copy link
Member

Trott commented Apr 26, 2018

we should fix the bug not document the bugged behaviour

@devsnek OK. #20323

I'd like to leave this PR open until that one lands.

@Tiriel
Copy link
Contributor

Tiriel commented Apr 26, 2018

As noted in #17128 (comment) the method still appears in the inspector-only methods (line 508 on the new diff). Maybe we should take the opportunity to fix this?

@Trott
Copy link
Member

Trott commented Apr 26, 2018

As noted in #17128 (comment) the method still appears in the inspector-only methods (line 508 on the new diff). Maybe we should take the opportunity to fix this?

@Tiriel #20346

@Trott
Copy link
Member

Trott commented Apr 28, 2018

99d56a4 landed so this can be closed.

Thanks for spotting the error @sant123 and for submitting a pull request for it!

@Trott Trott closed this Apr 28, 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. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants