New: `template-tag-spacing` rule (fixes #7631) #7913

jwilsson · 2017-01-12T20:44:02Z

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

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

Please describe what the rule should do:
This rule is very similiar to func-call-spacing in the way it will enforce spacing between template tags and their corresponding template literal.

What category of rule is this? (place an "X" next to just one item)

[ X ] Enforces code style
[ ] Warns about a potential error
[ ] Suggests an alternate way of doing something
[ ] Other (please specify:)

Provide 2-3 code examples that this rule will warn about:

/* eslint template-tag-spacing: ["error", "never"] */
func `foo`

/* eslint template-tag-spacing: ["error", "always"] */
func`foo`

Why should this rule be included in ESLint (instead of a plugin)?
Since it's really similar to func-call-spacing which is already included in ESLint it makes sense to enforce this styling too, especially since tagged template literals will become more and more common.

eslintbot · 2017-01-12T20:44:03Z

LGTM

mention-bot · 2017-01-12T20:44:03Z

@jwilsson, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @alberto and @kaicataldo to be potential reviewers.

not-an-aardvark

Thanks! This mostly looks good to me -- I just have a few small suggestions.

not-an-aardvark · 2017-01-12T20:48:23Z

docs/rules/template-tag-spacing.md

+
+(fixable) The `--fix` option on the [command line](../user-guide/command-line-interface#fix) automatically fixes problems reported by this rule.
+
+We can create functions whose parameters consist of template literals.


I think this line is slightly incorrect -- the function's arguments are the template strings and expressions, but the template literal itself doesn't get passed as an argument.

Yeah, you're right. I rewrote the whole first paragraph and added a "Further reading" section to explain the whole concept a bit better.

not-an-aardvark · 2017-01-12T20:54:17Z

lib/rules/template-tag-spacing.js

+         */
+        function checkSpacing(node) {
+            const tagToken = sourceCode.getLastToken(node.tag);
+            const literalToken = sourceCode.getTokenAfter(tagToken);


These lines will work incorrectly if the tag is surrounded by parens:

(foo)`bar`

As currently implemented, tagToken will refer to foo, and literalToken will refer to ), which isn't the intended behavior.

To fix this, I think it would be easier to get the tokens relative to the literal instead, since the literal can't be surrounded by parentheses.

const tagToken = sourceCode.getTokenBefore(node.quasi); const literalToken = sourceCode.getFirstToken(node.quasi);

@not-an-aardvark Won't the first line in your example also spit out the ) token? Should that be sourceCode.getFirstToken(node.tag)?

I'm not sure I understood you correctly, but I think we do want the ) token in this case -- we care about the spacing between the ) and the template, not the value inside the parens and the template.

// we care about this space ↓ ( foo ) `bar` // not this one ↑

not-an-aardvark · 2017-01-12T20:57:38Z

conf/eslint.json

@@ -231,6 +231,7 @@
        "strict": "off",
        "symbol-description": "off",
        "template-curly-spacing": "off",
+        "template-tag-spacing": "off",


Would you mind also adding this to packages/eslint-config-eslint/default.yml and turning it on? I think it would be useful to dogfood the rule in the ESLint codebase.

@not-an-aardvark Should that be in a separate (chore) PR?

I would be fine with either.

Good idea on the dog fooding! But agreed with @platinumazure that it should probably be a separate PR

I'd be happy to submit a chore PR once this one is finished.

not-an-aardvark · 2017-01-12T21:01:37Z

lib/rules/template-tag-spacing.js

+    meta: {
+        docs: {
+            description: "require or disallow spacing between template tags and their literals",
+            category: "ECMAScript 6",


I'm not sure, but I think we're generally trying to move stuff out of the "ECMAScript 6" category (also see eslint/archive-website#310). In my opinion, it might be better to put this in "Stylistic Issues".

"Stylistic issues" sounds good for me as well.

eslintbot · 2017-01-14T11:08:59Z

LGTM

jwilsson · 2017-01-14T11:11:12Z

I made the suggested changes. I rewrote the first paragraph of the documentation as well, to better explain what "tagged template literals" is.

nzakas · 2017-01-14T17:35:05Z

docs/rules/template-tag-spacing.md

+
+With ES6, it's possible to create functions called [tagged template literals](#further-reading) where the function parameters consist of a template literal's strings and expressions.
+
+This rule can force usage of spacing _between_ the tag function and the template literal.


This section shouldn't be about the rule, it should just be about the concept the rule relates to. So, can you remove this line and do a bit more explanation around what a tagged template is where spacing is allowed? I'd like to see a complete example with the function definition.

eslintbot · 2017-01-15T11:21:55Z

LGTM

jwilsson · 2017-01-15T11:22:47Z

@nzakas I updated the docs, I hope I understood you correctly.

nzakas

Good enough for me! (And I'll try to overlook the missing link to my book in Further Reading. ;) )

kaicataldo

Thanks for contributing! I have a few nitpicks about the documentation, and would like to request tests for when there's a comment between the tag and the template literal to make it clear what the behavior should be:

func/*here's a comment*/`Hello world`; // This should not warn
func /*here's a comment*/`Hello world`; // This should
func/*here's a comment*/ `Hello world`; // As should this

kaicataldo · 2017-01-16T06:17:45Z

docs/rules/template-tag-spacing.md

@@ -0,0 +1,78 @@
+# Enforce Usage of Spacing in Tagged Template Literals (template-tag-spacing)


Can you update this to match this description?

i.e. Require or disallow spacing between template tags and their literals (template-tag-spacing)

kaicataldo · 2017-01-16T06:19:06Z

docs/rules/template-tag-spacing.md

+}
+```
+
+This rule has one option which has either `"never"` or `"always"` as value.


Looks like there's a word missing here - maybe This rule has one option whose value can be set to "never" or "always"?

eslintbot · 2017-01-16T17:39:28Z

LGTM

jwilsson · 2017-01-16T17:42:33Z

@kaicataldo Done and done!

kaicataldo · 2017-01-17T03:45:33Z

tests/lib/rules/template-tag-spacing.js

+            options: ["always"]
+        },
+        {
+            code: "tag /*here's a comment*/`Hello world`",


Thanks for updating this! One final thing - this still gets autofixed so we do need an output property (even if it's just to assert that it doesn't get fixed). With the current logic, I believe it will remove the comment, correct? My preference would be to either figure out a way to keep the comment in the fixed output or to not fix if a comment exists (with a strong preference for the former), rather than simply remove the comment. Thoughts?

@not-an-aardvark Can you be a resource here? I think you've written more autofixes than the rest of us combined by now!

I think the best way to fix this would be to get a list of all the comments between the tag and the template literal, then concatenate them to get the resulting text between the two tokens. This would preserve the whitespace in the comments themselves and remove any other whitespace.

However, we should make sure we don't change the behavior of this code:

foo // line comment `bar`

The implementation I mentioned above would fix that to

foo// line comment`bar`

To avoid this, I think we should avoid doing a fix if there are any line comments between the tag and the template literal.

So I would do something along the lines of

context.report({ // ... fix(fixer) { const comments = sourceCode.getComments(node.quasi).leading; if (comments.some(comment => comment.type === "Line")) { return null; } return fixer.replaceTextRange( [tagToken.range[1], literalToken.range[0]], comments.reduce((text, comment) => text + sourceCode.getText(comment), "") ); } });

Ah, of course. Sorry about that!

But I'm guessing we shouldn't try to fix anything in cases like this:

/* eslint template-tag-spacing: ["error", "always"] */ tag/*here's a comment*/`Hello world`

Since we wouldn't know where to put the space.

@not-an-aardvark Thanks for the explanation and example!

@jwilsson That seems reasonable. Another option would be to just always put the space immediately after the tag.

Alright, I added the expected output to all tests and opted for the "space immediately after tag" approach when autofixing comments.

Rewrote the intro do be more descriptive about tagged template literals. Also includes a "Further Reading" section.

eslintbot · 2017-01-21T12:52:26Z

LGTM

kaicataldo

This looks great to me. Thanks for contributing to ESLint!

not-an-aardvark · 2017-01-27T02:27:39Z

Thanks for contributing!

not-an-aardvark suggested changes Jan 12, 2017

View reviewed changes

not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules labels Jan 12, 2017

nzakas requested changes Jan 14, 2017

View reviewed changes

not-an-aardvark approved these changes Jan 15, 2017

View reviewed changes

nzakas approved these changes Jan 15, 2017

View reviewed changes

kaicataldo suggested changes Jan 16, 2017

View reviewed changes

kaicataldo suggested changes Jan 17, 2017

View reviewed changes

jwilsson added 8 commits January 21, 2017 09:17

New: template-tag-spacing rule (fixes #7631)

ec14157

Check template tags wrapped in parens

f83481c

Change rule category to "Stylistic Issues"

cd8fd2c

Rephrase rule documentation intro

8c1bb50

Rewrote the intro do be more descriptive about tagged template literals. Also includes a "Further Reading" section.

Update docs to better explain the rule concept

d001c13

Add tests for tags with comments

f24deb3

Documentation nits

72fb1ab

Adjust template tag autofixing behaviour when there's comments present

4699b52

kaicataldo approved these changes Jan 21, 2017

View reviewed changes

ilyavolodin approved these changes Jan 23, 2017

View reviewed changes

not-an-aardvark merged commit 09546a4 into eslint:master Jan 27, 2017

This was referenced Feb 3, 2017

Update dependency eslint to version 3.15.0 singapore/lint-condo#231

Merged

Update dependency eslint to version 3.15.0 renovatebot/renovate#88

Merged

This was referenced Feb 5, 2017

[Chore] Update dependency eslint to version 3.15.0 SimpliField/sf-preferences#34

Merged

[Chore] Update dependency eslint to version 3.15.0 SimpliField/eslint-config-simplifield#49

Merged

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: `template-tag-spacing` rule (fixes #7631) #7913

New: `template-tag-spacing` rule (fixes #7631) #7913

jwilsson commented Jan 12, 2017

eslintbot commented Jan 12, 2017

mention-bot commented Jan 12, 2017

not-an-aardvark left a comment

not-an-aardvark Jan 12, 2017

jwilsson Jan 14, 2017

not-an-aardvark Jan 12, 2017

platinumazure Jan 12, 2017

not-an-aardvark Jan 12, 2017 •

edited

not-an-aardvark Jan 12, 2017

platinumazure Jan 12, 2017

not-an-aardvark Jan 12, 2017

vitorbal Jan 13, 2017

jwilsson Jan 14, 2017

not-an-aardvark Jan 12, 2017

vitorbal Jan 13, 2017

eslintbot commented Jan 14, 2017

jwilsson commented Jan 14, 2017

nzakas Jan 14, 2017

eslintbot commented Jan 15, 2017

jwilsson commented Jan 15, 2017

nzakas left a comment

kaicataldo left a comment •

edited

kaicataldo Jan 16, 2017

kaicataldo Jan 16, 2017

eslintbot commented Jan 16, 2017

jwilsson commented Jan 16, 2017

kaicataldo Jan 17, 2017

not-an-aardvark Jan 17, 2017

jwilsson Jan 17, 2017

not-an-aardvark Jan 19, 2017

jwilsson Jan 21, 2017

eslintbot commented Jan 21, 2017

kaicataldo left a comment

not-an-aardvark commented Jan 27, 2017


		(fixable) The `--fix` option on the [command line](../user-guide/command-line-interface#fix) automatically fixes problems reported by this rule.

		We can create functions whose parameters consist of template literals.


		With ES6, it's possible to create functions called [tagged template literals](#further-reading) where the function parameters consist of a template literal's strings and expressions.

		This rule can force usage of spacing _between_ the tag function and the template literal.

		@@ -0,0 +1,78 @@
		# Enforce Usage of Spacing in Tagged Template Literals (template-tag-spacing)

New: template-tag-spacing rule (fixes #7631) #7913

New: template-tag-spacing rule (fixes #7631) #7913

Conversation

jwilsson commented Jan 12, 2017

eslintbot commented Jan 12, 2017

mention-bot commented Jan 12, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-an-aardvark Jan 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Jan 14, 2017

jwilsson commented Jan 14, 2017

Choose a reason for hiding this comment

eslintbot commented Jan 15, 2017

jwilsson commented Jan 15, 2017

nzakas left a comment

Choose a reason for hiding this comment

kaicataldo left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Jan 16, 2017

jwilsson commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Jan 21, 2017

kaicataldo left a comment

Choose a reason for hiding this comment

not-an-aardvark commented Jan 27, 2017

New: `template-tag-spacing` rule (fixes #7631) #7913

New: `template-tag-spacing` rule (fixes #7631) #7913

not-an-aardvark Jan 12, 2017 •

edited

kaicataldo left a comment •

edited