util, test: improve inspect escaping#21624
Conversation
Right now util.inspect will always escape single quotes. That is not necessary though in case the string that will be escaped does not contain double quotes. In that case the string can simply be wrapped in double quotes instead.
This reduces the amount of quotes util.inspect escapes by falling back to backticks in case a string contains single and double quotes. That makes sure only very few strings have to escape quotes at all. Thus it increases the readability of these strings.
The string escaping is hard to read. This changes all those escaped strings to use double quotes instead so no escaping is necessary.
nodejs-github-bot
added
the
util
BridgeAR
added
the
semver-major
ChALkeR
left a comment
ChALkeR
left a comment
There was a problem hiding this comment.
> console.log(util.inspect('${xx}\'"'))
`${xx}'"`
undefined
> `${xx}'"`
ReferenceError: xx is not definedIf $ is present backticks should be avoided as well.
Also, please add a test for that.
|
@ChALkeR done |
|
Ping @ChALkeR |
|
@nodejs/tsc PTAL. @ChALkeR PTAL, I addressed your comment :) |
|
@BridgeAR Sorry for the delay, totally missed this. 😞 |
|
I'm OK with the change |
|
I'm neutral on this one. I see that there is benefit. And I see that there is maintenance cost. And I wonder whether or not it's an issue that people may be surprised. ("Why did I get a template string one time, a single-quoted string the other time, and a double-quoted string the last time? What is going on?!") Honestly have no idea if that will ever come up and if it's a problem or not. Leaving a note so that it doesn't feel like the TSC ping is being ignored. |
|
@targos is the "OK" a LG? In that case I would go ahead and land this. @Trott I believe that is relatively clear from the output itself: if the string contains a single quote it is clear that it can only be wrapped into double quotes or back ticks or the single quote would have to be escaped. Do you really think there is room for confusion? I actually wonder if people will even notice the difference besides being able to read the output without knowing that it was escaped before. |
|
@BridgeAR consider it a rubber-stamp LG. I didn't review the code. |
|
CI before landing: https://ci.nodejs.org/job/node-test-pull-request/15893/ |
Right now util.inspect will always escape single quotes. That is not
necessary though in case the string that will be escaped does not
contain double quotes. In that case the string can simply be wrapped
in double quotes instead.
If the string contains single and double quotes and it does not
contain `${` as part of the string, backticks will be used instead.
That makes sure only very few strings have to escape quotes at all.
Thus it increases the readability of these strings.
PR-URL: nodejs#21624
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
The string escaping is hard to read. This changes all those escaped strings to use double quotes instead so no escaping is necessary. PR-URL: nodejs#21624 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
The string escaping is hard to read. This changes all those escaped strings to use double quotes instead so no escaping is necessary. PR-URL: #21624 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
7 participants
For me it is always hard to read strings that use escaped quotes. This PR reduces the amount of quotes that have to be escaped significantly be just using either double quotes or backticks instead, if appropriate. That should improve the readability of these strings a lot.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes