Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: addallow option forrestrict-template-expressions#8556

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

abrahamguo
Copy link
Contributor

@abrahamguoabrahamguo commentedFeb 27, 2024•
edited by JoshuaKGoldberg
Loading

PR Checklist

Overview

Building on the work from#8389, which@JoshuaKGoldberg has been reviewing, this PR ports theignoredTypeNames option fromno-base-to-string intorestrict-template-expressions. I'm open to further discussion about this, as this update definitely blurs the line between the two rules.

@typescript-eslint
Copy link
Contributor

Thanks for the PR,@abrahamguo!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently onhttps://opencollective.com/typescript-eslint.

@netlifyNetlify
Copy link

netlifybot commentedFeb 27, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commitd8509be
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/66d62c16df293c0008bcc03d
😎 Deploy Previewhttps://deploy-preview-8556--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse		1 paths audited
Performance: 99 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 90 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to yourNetlify site configuration.

@nx-cloudNx Cloud
Copy link

nx-cloudbot commentedFeb 27, 2024•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commitd8509be. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 30 targets

Sent with 💌 fromNxCloud.

@JoshuaKGoldbergJoshuaKGoldberg requested review frombradzacher and removed request forbradzacherMarch 18, 2024 12:56
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Thanks for sending this in - but would you be open to filing an issue? I think we'd want to discuss this a bit before landing it.

Also, just a couple preliminary review thoughts if it does land:

  • Unit tests?
  • I think we'd want to share the default value ofignoredTypeNames in a constant so it doesn't risk getting out of sync

@JoshuaKGoldbergJoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party labelMar 18, 2024
@JoshuaKGoldberg
Copy link
Member

Marking as a draft pending#8722's resolution.

@bradzacherbradzacher removed the awaiting responseIssues waiting for a reply from the OP or another party labelAug 25, 2024
JoshuaKGoldberg
JoshuaKGoldberg previously approved these changesAug 25, 2024
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment•
edited
Loading

Choose a reason for hiding this comment

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

This is fantastic, great work@abrahamguo! Really clean implementation and I like the sharing of docs. Thanks!

I'll also want to wait on another review from @typescript-eslint/triage-team. The more we use the sharedTypeOrValueSpecifier format the more review eyes we'll want on it.

IMO we should land this before#9875 and then I can adjust the docs there to fit this PR.

Captain Holt in Brooklyn 99 Happily clapping his hands together and shouting 'Hot damn!' with fiery steel caption font

abrahamguo reacted with heart emoji
@JoshuaKGoldbergJoshuaKGoldberg requested a review froma teamAugust 25, 2024 16:40
@JoshuaKGoldbergJoshuaKGoldberg added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labelAug 25, 2024
Copy link
Member

@kirkwaiblingerkirkwaiblinger left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Since#9875 ended up landing first, some merge conflicts will need to be addressed. Ping for another review pass after that is completed?

@kirkwaiblingerkirkwaiblinger added the awaiting responseIssues waiting for a reply from the OP or another party labelAug 30, 2024
@github-actionsgithub-actionsbot removed 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge awaiting responseIssues waiting for a reply from the OP or another party labelsSep 2, 2024
@abrahamguo
Copy link
ContributorAuthor

Conflicts resolved and ready for re-review!

I am not clear why the one test is failing. It works locally for me.

@JoshuaKGoldberg
Copy link
Member

It's from the CI job that runs tests with the project service enabled:

TYPESCRIPT_ESLINT_PROJECT_SERVICE:true

Copying the failure here:

  ● restrict-template-expressions › valid › const msg = `arg = ${new URLSearchParams()}`;    assert.strictEqual(received, expected)    Expected value to strictly be equal to:      0    Received:      1        Message:      Should have no errors but had 1: [      {        ruleId: '@rule-tester/restrict-template-expressions',        severity: 2,        message: 'Invalid type "URLSearchParams" of template literal expression.',        line: 1,        column: 22,        nodeType: 'NewExpression',        messageId: 'invalidType',        endLine: 1,        endColumn: 43      }    ]

You can directly test that file inpackages/eslint-plugin withTYPESCRIPT_ESLINT_PROJECT_SERVICE=true yarn jest restrict-template-.

abrahamguo reacted with eyes emoji

@abrahamguo
Copy link
ContributorAuthor

Hmm, looks like settingTYPESCRIPT_ESLINT_PROJECT_SERVICE=true causes it to bypass thetsconfig-withdom.json set inrestrict-template-expressions.test.ts.

@abrahamguo
Copy link
ContributorAuthor

I've worked around this by ensuring that (similar to the new examples in the docs page) the unit tests only refer to theError type, which is available with or without theDOM types.
URL andURLSearchParams are not shown in the docs code examples, nor in the unit tests, since they requireDOM types in order to work properly.

Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

✨

@JoshuaKGoldbergJoshuaKGoldberg added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labelSep 13, 2024
@@ -85,7 +85,7 @@ const msg2 = `arg = ${arg || 'not truthy'}`;

### `allowAny`

Whether to `any` typed values in template expressions.

Choose a reason for hiding this comment

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

lol

JoshuaKGoldberg reacted with laugh emoji
Copy link
Member

@kirkwaiblingerkirkwaiblinger left a comment

Choose a reason for hiding this comment

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

Peachy keen!

@JoshuaKGoldbergJoshuaKGoldberg merged commitaf92611 intotypescript-eslint:mainSep 16, 2024
61 checks passed
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsSep 25, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg approved these changes

@kirkwaiblingerkirkwaiblingerkirkwaiblinger approved these changes

Assignees
No one assigned
Labels
1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhancement: [restrict-template-expressions] More permissive type check
4 participants
@abrahamguo@JoshuaKGoldberg@kirkwaiblinger@bradzacher
[8]ページ先頭
©2009-2025 Movatter.jp