lib, test: minor console changes #20960
Conversation
The cli table used multi line template strings which are normally not used in our code base and it also upper cased a regular function name. This is changed by this patch.
The former error output was not readable in case of an error. This improves it by splitting the lines and therefore creating a nice and readable diff.
trivikr
added
test
|
PTAL |
BridgeAR
added
the
author ready
|
cc @devsnek |
lib/internal/cli_table.js
Outdated
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| const { Buffer } = require('buffer'); | |||
| const { removeColors } = require('internal/util'); | |||
| const HasOwnProperty = Function.call.bind(Object.prototype.hasOwnProperty); | |||
| const hasOwnProperty = Function.call.bind(Object.prototype.hasOwnProperty); | |||
There was a problem hiding this comment.
I know @bmeck has also been doing it this way, (at least guessing from the reasons that I use it) that it matches the name the spec uses?
There was a problem hiding this comment.
@BridgeAR are you attached to this particular change? It's the only thing stopping me from approving & landing to be honest and I've looked at this PR several times. I believe we should have a convention and follow it — at the moment uppercase is the way we do this in most files. I'm not opposed to switching but I don't think it should be done incrementally.
There was a problem hiding this comment.
I switched it back even though this is really weird for me as it is not a class. IMO we should only have upper case characters for classes.
|
And again: https://ci.nodejs.org/job/node-test-pull-request/15873/ |
|
@BridgeAR Can you give an example of value for which the output is different with this PR? I'd like to test it. |
|
@targos the test output is now readable in case of a failure in |
|
@BridgeAR Is the output of Edit: Confirmed it by looking more closely at the code. |
The cli table used multi line template strings which are normally not used in our code base and it also upper cased a regular function name. This is changed by this patch. PR-URL: nodejs#20960 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
The former error output was not readable in case of an error. This improves it by splitting the lines and therefore creating a nice and readable diff. PR-URL: nodejs#20960 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos
removed
the
author ready
The cli table used multi line template strings which are normally not used in our code base and it also upper cased a regular function name. This is changed by this patch. PR-URL: #20960 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
The former error output was not readable in case of an error. This improves it by splitting the lines and therefore creating a nice and readable diff. PR-URL: #20960 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
I was working on console a bit and I decided to split my work into smaller chunks. This is just a minor refactoring of the cli table and it improves the error output in case of an error for console table.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes