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
merged 1 commit into from Nov 27, 2019
Merged

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

merged 1 commit into from Nov 27, 2019

Conversation

bradzacher
Copy link
Contributor

@bradzacher 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-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon 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 ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels May 10, 2019
@mysticatea
Copy link
Member

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?

@platinumazure
Copy link
Member

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

platinumazure
platinumazure previously approved these changes Jun 8, 2019
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mysticatea
Copy link
Member

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

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 November 21, 2019 21:34

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

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
Copy link
Contributor Author

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 There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Nov 22, 2019
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for contributing!

Copy link
Member

@ilyavolodin ilyavolodin left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for updating it!

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators May 27, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label May 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants