Skip to content

Formatting: fix unwanted space between closing parenthesis and string template#23697

Merged
mhegazy merged 2 commits into
microsoft:masterfrom
Ken703:bug/23047
Apr 30, 2018
Merged

Formatting: fix unwanted space between closing parenthesis and string template#23697
mhegazy merged 2 commits into
microsoft:masterfrom
Ken703:bug/23047

Conversation

@Ken703
Copy link
Copy Markdown
Contributor

@Ken703 Ken703 commented Apr 26, 2018

Fixes #23047

@msftclas
Copy link
Copy Markdown

msftclas commented Apr 26, 2018

CLA assistant check
All CLA requirements met.

Comment thread src/services/formatting/rules.ts Outdated
rule("NoSpaceBetweenTagAndTemplateString", SyntaxKind.Identifier, [SyntaxKind.NoSubstitutionTemplateLiteral, SyntaxKind.TemplateHead], [isNonJsxSameLineTokenContext], RuleAction.Delete),

// No space between closing parenthesis and template string
rule("NoSpaceBetweenCloseParenAndTemplateString", SyntaxKind.CloseParenToken, SyntaxKind.NoSubstitutionTemplateLiteral, [isNonJsxSameLineTokenContext], RuleAction.Delete),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@mhegazy should this just be folded into the above rule?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes please. it should be something like:

rule("NoSpaceBetweenTagAndTemplateString", [SyntaxKind.Identifier, SyntaxKind.CloseParenToken], [SyntaxKind.NoSubstitutionTemplateLiteral, SyntaxKind.TemplateHead], [isNonJsxSameLineTokenContext], RuleAction.Delete),

format.document();
verify.currentFileContentIs(
`foo()\`abc\`;
bar()\`def\`;`
Copy link
Copy Markdown
Contributor

@mhegazy mhegazy Apr 26, 2018

Choose a reason for hiding this comment

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

also add a test for a template with substitutions, since these are parsed differently e.g.

baz()`a${x}b`;

@DanielRosenwasser
Copy link
Copy Markdown
Member

Just as a heads up, your commits don't seem to be associated with your GitHub account. While this isn't technically a problem, you might care if you want more appropriate attribution. You can either make sure your GitHub account is associated with the email address you're using for your commits, or rebase and amend your commits to fix the author name and email.

@mhegazy mhegazy merged commit 8793b8c into microsoft:master Apr 30, 2018
@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Apr 30, 2018

thanks @Ken703!

@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants