Chore: refactor template literal feature detection in 'quotes' rule. #11125
Conversation
eslint-deprecated
bot
added
the
triage
bmish
force-pushed
the
quotes-rule-refactor-template-literal-feature-detection
branch
3 times, most recently
from
1cbb105
to
b83d5e5
Compare
lib/rules/quotes.js
Outdated
| @@ -228,6 +228,34 @@ module.exports = { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Checks whether or not a given TemplateLiteral node is using a template literal feature. | |||
There was a problem hiding this comment.
it seems confusing to me. all templateLiteral nodes are using template literal feature, no?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think maybe the word "feature" is confusing here -- I've seen these referred to as "template expressions".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
In that case I'm also confused -- what three features are you referring to?
There was a problem hiding this comment.
The features are:
- Expression interpolation
- Tagged templates
- 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.
There was a problem hiding this comment.
Is there still confusion around this? Happy to make improvements.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
added
rule
|
Could you please share what problem you're trying to solve? What motivated you to make this change? |
|
@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
force-pushed
the
quotes-rule-refactor-template-literal-feature-detection
branch
from
b83d5e5
to
2df365b
Compare
bmish
force-pushed
the
quotes-rule-refactor-template-literal-feature-detection
branch
from
2df365b
to
35131ba
Compare
platinumazure
added
accepted
eslint-deprecated
bot
added
the
archived due to age
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.