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

Fix: Add support for tagged template literal generics in no-unexpected-multiline #11698

Merged

Conversation

@bradzacher
Copy link
Contributor

bradzacher commented May 10, 2019

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

  • Bug fix

#11650

What changes did you make? (Give an overview)
Adds support for multiline tagged template expressions with generics (typescript only)

tag<
  generic
>`string`

I thought that because this change was so simple, and it is a no-op for non-ts code, it might be a good fit to fix upstream instead of creating a new rule in the typescript plugin.
Don't be afraid to reject if you don't think it fits.

@eslint eslint bot added the triage label May 10, 2019
@g-plane g-plane changed the title fix(no-unexpected-multiline): Add support for tagged template literal generics Fix: Add support for tagged template literal generics in no-unexpected-multiline May 10, 2019
@g-plane g-plane added bug evaluating rule and removed triage labels May 10, 2019
@mysticatea
Copy link
Member

mysticatea commented May 10, 2019

Thank you for this PR.

I don't intend to block this PR, but maybe ESTree should be updated because TaggedTemplateExpression doesn't have type annotation. (Or @typescript-eslint make the own AST specification document to share the spec.)

Would you change the commit message to follow our contributing guide?

@bradzacher bradzacher force-pushed the bradzacher:11650-multiline-tagged-template branch 2 times, most recently from e516ac0 to f988366 May 10, 2019
@platinumazure
Copy link
Member

platinumazure commented Jun 8, 2019

@mysticatea Do you have concerns with merging this PR even if ESTree isn't updated?

Copy link
Member

platinumazure left a comment

LGTM, thanks!

@platinumazure platinumazure requested a review from ilyavolodin Jun 8, 2019
@mysticatea
Copy link
Member

mysticatea commented Jun 8, 2019

Related discussion is here: typescript-eslint/typescript-eslint#510

I'm happy if we can review based on the AST spec.

@ilyavolodin
Copy link
Member

ilyavolodin commented Jun 8, 2019

I think I'm with @mysticatea here. You are using a property that is not part of the official spec, and while it doesn't hurt anyone, it makes it harder for others to contribute to this rule later. I would want to find some other way to support this feature, without undocumented properties.

@platinumazure platinumazure dismissed their stale review Nov 21, 2019

Agree with @ilyavolodin that we should try to avoid using undocumented properties. I would be in favor of a PR that doesn't use TS-specific node properties.

@platinumazure
Copy link
Member

platinumazure commented Nov 21, 2019

The TSC discussed this. I came up with this suggestion for an alternative approach-- no idea if it will work, but worth a try maybe?

It seems like it should be possible to use the previous (non-comment) token to the opening template literal backtick, and see if that's on the same line as the backtick. That would work for both a generic > and for a template tag, right?

I guess another way to word my suggestion is-- let's have the "template tag" (for the purpose of that rule) be the tokens from the tag identifier, through the last token before the backtick. Rather than only the tag identifier

For a pure JS tagged template, the "template tag" tokens would be just one token, the tag identifier

@bradzacher If you happen to get back to this-- could you please try that approach and see if it could be made to work? Then that would let us avoid mentioning TS-specific node type or node properties and it would hopefully also work for the Typescript use case. Thanks!

@bradzacher bradzacher force-pushed the bradzacher:11650-multiline-tagged-template branch from f988366 to d4cb8e7 Nov 22, 2019
@bradzacher bradzacher force-pushed the bradzacher:11650-multiline-tagged-template branch from d4cb8e7 to cff3a82 Nov 22, 2019
@bradzacher
Copy link
Contributor Author

bradzacher commented Nov 22, 2019

updated as requested

@btmills
Copy link
Member

btmills commented Nov 22, 2019

Thanks for the update, @bradzacher! Now that this doesn't use undocumented node properties, I'll mark it as accepted and give the team a chance to look it over.

@btmills btmills added accepted and removed evaluating labels Nov 22, 2019
Copy link
Member

platinumazure left a comment

Looks good to me. Thanks for contributing!

Copy link
Member

ilyavolodin left a comment

LGTM. Thanks for updating it!

@platinumazure platinumazure merged commit ea16de4 into eslint:master Nov 27, 2019
18 checks passed
18 checks passed
Verify Files
Details
Test (ubuntu-latest, 13.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 8.10.0)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message Commit message follows guidelines
Details
continuous-integration Build #20191122.3 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@bradzacher bradzacher deleted the bradzacher:11650-multiline-tagged-template branch Nov 27, 2019
@eslint eslint bot locked and limited conversation to collaborators May 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.