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

util, test: improve inspect escaping #21624

Closed
wants to merge 4 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Jul 2, 2018

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), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

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 nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Jul 2, 2018
@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 2, 2018
@BridgeAR BridgeAR requested a review from a team July 2, 2018 21:50
ChALkeR
ChALkeR previously requested changes Jul 3, 2018
Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

> console.log(util.inspect('${xx}\'"'))
`${xx}'"`
undefined
> `${xx}'"`
ReferenceError: xx is not defined

If $ is present backticks should be avoided as well.
Also, please add a test for that.

@BridgeAR
Copy link
Member Author

BridgeAR commented Jul 4, 2018

@ChALkeR done

@BridgeAR
Copy link
Member Author

BridgeAR commented Jul 4, 2018

@BridgeAR
Copy link
Member Author

BridgeAR commented Jul 9, 2018

Ping @ChALkeR
@nodejs/tsc PTAL

@BridgeAR
Copy link
Member Author

@nodejs/tsc PTAL.

@ChALkeR PTAL, I addressed your comment :)

@BridgeAR BridgeAR requested a review from a team July 13, 2018 15:19
@ChALkeR
Copy link
Member

ChALkeR commented Jul 13, 2018

@BridgeAR Sorry for the delay, totally missed this. 😞
I dismissed my stale review.

@targos
Copy link
Member

targos commented Jul 14, 2018

I'm OK with the change

@Trott
Copy link
Member

Trott commented Jul 15, 2018

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.

@BridgeAR
Copy link
Member Author

@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.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@targos
Copy link
Member

targos commented Jul 16, 2018

@BridgeAR consider it a rubber-stamp LG. I didn't review the code.

@BridgeAR
Copy link
Member Author

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jul 16, 2018
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jul 16, 2018
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@BridgeAR
Copy link
Member Author

Landed in b3e93a9, 9c50199 🎉

@BridgeAR BridgeAR closed this Jul 16, 2018
targos pushed a commit that referenced this pull request Jul 31, 2018
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@targos targos mentioned this pull request Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants