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

Chore: refactor template literal feature detection in 'quotes' rule. #11125

Conversation

@bmish
Copy link
Contributor

commented Nov 25, 2018

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[X] Other, please explain:

What changes did you make? (Give an overview)

No behavior change. This change is a refactoring that extracts a helper function for detecting the usage of a template literal feature (which is used to determine if a template literal should be allowed by the 'quotes' rule). This refactoring separates that detection logic from the rest of the 'quotes' rule logic.

Is there anything you'd like reviewers to focus on?

Nothing specifically.

@eslint eslint bot added the triage label Nov 25, 2018
@bmish bmish force-pushed the bmish:quotes-rule-refactor-template-literal-feature-detection branch 3 times, most recently from 1cbb105 to b83d5e5 Nov 26, 2018
@@ -228,6 +228,34 @@ module.exports = {
}
}

/**
* Checks whether or not a given TemplateLiteral node is using a template literal feature.

This comment has been minimized.

Copy link
@aladdin-add

aladdin-add Nov 27, 2018

Member

it seems confusing to me. all templateLiteral nodes are using template literal feature, no?

This comment has been minimized.

Copy link
@bmish

bmish Nov 27, 2018

Author Contributor

For example, the TemplateLiteral node `hello world` is not using a template literal feature, whereas `hello ${name}` is using one. This matters because the 'quotes' rule may want to disallow an unnecessary template literal from being used, when a simple string literal 'hello world' would suffice.

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Nov 27, 2018

Member

I think maybe the word "feature" is confusing here -- I've seen these referred to as "template expressions".

This comment has been minimized.

Copy link
@bmish

bmish Nov 27, 2018

Author Contributor

Yeah, my example in the last comment is using a template expression. But expressions are just one feature of template literals. This function is checking if any of the three template literal features are in use.

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Nov 27, 2018

Member

In that case I'm also confused -- what three features are you referring to?

This comment has been minimized.

Copy link
@bmish

bmish Nov 27, 2018

Author Contributor

The features are:

  1. Expression interpolation
  2. Tagged templates
  3. Multi-line strings (linebreaks)

The official documentation mentions these too: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals

Note that the existing code here was checking for these same features, but I'm trying to make these checks clearer by moving them into a helper function.

This comment has been minimized.

Copy link
@bmish

bmish Nov 30, 2018

Author Contributor

Is there still confusion around this? Happy to make improvements.

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Dec 1, 2018

Member

It might be a good idea to list the three features explicitly in the comment since it seems like the word "feature" has the potential to cause confusion. Aside from that, this change looks good to me.

This comment has been minimized.

Copy link
@bmish

bmish Dec 1, 2018

Author Contributor

Have another look. I updated the wording of the comment and the function name. Hope this helps. (I was trying to avoid listing the features in the comment as that would be unnecessary repetition of what's in the code directly below).

@aladdin-add aladdin-add added rule evaluating chore and removed triage labels Nov 27, 2018
@platinumazure

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

Could you please share what problem you're trying to solve? What motivated you to make this change?

@bmish

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2018

@platinumazure thanks for the question. There's no problem. My goal was just to improve the code quality with a small refactoring. It came up because I was thinking about rule improvements that I could make in this area, and I thought this refactoring would make future changes easier.

@bmish bmish force-pushed the bmish:quotes-rule-refactor-template-literal-feature-detection branch from b83d5e5 to 2df365b Dec 1, 2018
@bmish bmish force-pushed the bmish:quotes-rule-refactor-template-literal-feature-detection branch from 2df365b to 35131ba Dec 1, 2018
Copy link
Member

left a comment

LGTM, thanks!

@platinumazure platinumazure added accepted and removed evaluating labels Dec 7, 2018
Copy link
Member

left a comment

LGTM, thanks for contributing!

@not-an-aardvark not-an-aardvark merged commit dd7b0cb into eslint:master Dec 8, 2018
5 checks passed
5 checks passed
commit-message Commit message follows guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@eslint eslint bot locked and limited conversation to collaborators Jun 7, 2019
@eslint eslint bot added the archived due to age label Jun 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.