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

doc, lib, test, tools: eliminate quote escaping #20214

Closed
wants to merge 1 commit into from
Closed

doc, lib, test, tools: eliminate quote escaping #20214

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 22, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

Currently, we have ESLint rule that allows using double quotes to reduce escaping of single quotes inside strings.

However, we have some flaws in this domain:

  • This possibility is not fully utilized in our code.
  • The ESLint rule does not suffice for cases when both single and double quotes are used inside strings.

This PR tries to fix both:

  • The ESLint rule is expanded to allow backticks, to reduce escaping in strings with both single and double quotes inside.
  • Most of the current escaping cases are eliminated.

It seems this change improves readability and sometimes reduces string concatenating due to a shorter length of fixed strings.

Refs: https://eslint.org/docs/rules/quotes

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 22, 2018
@vsemozhetbyt vsemozhetbyt added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Apr 22, 2018
@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt
Copy link
Contributor Author

It seems this should only be backported to v10 (easy) and v8 (not so hard): v6 is almost on maintenance and v9 will be EOL soon. I can do both backportings then (proactively if requested).

@devsnek
Copy link
Member

devsnek commented Apr 22, 2018

this seems pretty large and i think escaping looks better, its probably fine how it is?

@vsemozhetbyt
Copy link
Contributor Author

Let's see what others think, we can easily close this)

@devsnek
Copy link
Member

devsnek commented Apr 22, 2018

@nodejs/linting

Copy link
Contributor

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

¯\_(ツ)_/¯ I have no strong feelings either way.

@Trott
Copy link
Member

Trott commented Apr 23, 2018

Not a strong feeling, but I tend to side with @devsnek that this is a fair amount of churn that doesn't seem to substantially improve the code. But if others feel differently, I'm fine with it. I would ask that if you intend to land it on master soon, that you open backport PRs right away because it seems highly likely it will not cleanly cherry-pick to the staging branches.

@Trott
Copy link
Member

Trott commented Apr 23, 2018

Actually, upon further review, I'm a little concerned by allowTemplateLiterals. In the past (before linting prevented it), I believe there had been nits about using backticks unnecessarily and I don't want to see those return if our linter becomes more lenient than our reviewers. I might be -1 on this after all....

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Yeah, sorry, but I gotta -1 this, at least as long as allowTemplateLiterals is in there. With this change, this becomes acceptable to the linter:

var http = require(`http`);  // using backticks instead of single quotes!

I imagine I'm not the only one that wouldn't want to make the linter more lenient that way.

@vsemozhetbyt
Copy link
Contributor Author

Ok, I will close due to doubts and concerns)

@vsemozhetbyt vsemozhetbyt deleted the tools-eslint-quotes-backticks branch April 23, 2018 06:24
@BridgeAR
Copy link
Member

@vsemozhetbyt when looking at these changes, it feels like 95% were not about using backticks and just improved the situation by switching to double quotes instead of having escaped single quotes in single quotes.

@vsemozhetbyt
Copy link
Contributor Author

@BridgeAR Yeah, but there were also shared concerns if this actually makes the code better, and I do not want to make collaborators disturbed over heavy nits without obvious improvements)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants