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

test(eslint-plugin): adjust tests to verifyno-unnecessary-type-assertion doesn't report template literals with expressions#10671

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

ronami
Copy link
Member

@ronamironami commentedJan 17, 2025•
edited
Loading

PR Checklist

Overview

I've noticed this while reviewing#10618.

To my understanding, these tests should make sure the ruledoesn't report on template literals with expressions.

It seems that even if this check is removed, these tests still pass. I think it's related to "fresh literals" (more on this here), as is shown below (playground link):

constfoo='foo';// the `hello ${foo}` expression does count as a literalconsttest1=`hello${foo}`asconst;//      ^?letbar='bar';// the `hello ${bar}` expression doesn't count as a literal// as it's inferred as `hello ${string}`consttest2=`hello${bar}`asconst;//      ^?

The expression coming from a widenedlet variabledoesn't count as a literal, and it falls back tobeing checked as a non-literal (which always allowsas const assertions).

This PR adjusts the tests to fail if this check is removed or changed incorrectly (this seems correctaccording to the original issue).

@typescript-eslint
Copy link
Contributor

Thanks for the PR,@ronami!

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 commentedJan 17, 2025•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commit27ba6f7
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/678d5f3ef9e6a000087344dd
😎 Deploy Previewhttps://deploy-preview-10671--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 (🟢 up 1 from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 98 (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 commentedJan 17, 2025•
edited
Loading

View yourCI Pipeline Execution ↗ for commit27ba6f7.

CommandStatusDurationResult
nx run-many --target=build --exclude website --...✅ Succeeded3sView ↗
nx run-many --target=clean✅ Succeeded10sView ↗

☁️Nx Cloud last updated this comment at2025-01-20 09:31:54 UTC

@ronamironami changed the titletest(eslint-plugin): adjust tests to verifyno-unnecessary-type-assertion doesn't throw on template literals with expressionstest(eslint-plugin): adjust tests to verifyno-unnecessary-type-assertion doesn't report template literals with expressionsJan 17, 2025
@ronamironami marked this pull request as ready for reviewJanuary 17, 2025 14:36
@kirkwaiblinger
Copy link
Member

should we just usedeclare let/declare const to sidestep any complexity about the possible type?

ronami reacted with thumbs up emoji

@kirkwaiblingerkirkwaiblinger added the awaiting responseIssues waiting for a reply from the OP or another party labelJan 19, 2025
@ronami
Copy link
MemberAuthor

ronami commentedJan 19, 2025•
edited
Loading

should we just usedeclare let/declare const to sidestep any complexity about the possible type?

Hmm since these two are different types (one is a "fresh literal", the other isn't):

// not a "fresh literal"declareconsta:'foo';// is a "fresh literal"constb='foo';

Along with seeing how much complexity there is around "fresh literals", I've changed both tests to usedeclare const but also added one test for a standard "fresh literal"const key = ... usage. I'm not sure if it's really necessary though, I'm totally OK removing it too.

Wdyt?

Edit: I manually removed theawaiting response label, as I think there's no review to ask for a re-review.

kirkwaiblinger reacted with thumbs up emojikirkwaiblinger reacted with rocket emoji

@ronamironami removed the awaiting responseIssues waiting for a reply from the OP or another party labelJan 19, 2025
@codecovCodecov
Copy link

codecovbot commentedJan 19, 2025•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.15%. Comparing base(31be053) to head(27ba6f7).
Report is 13 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@##             main   #10671   +/-   ##=======================================  Coverage   87.15%   87.15%           =======================================  Files         448      448             Lines       15576    15578    +2       Branches     4551     4551           =======================================+ Hits        13575    13577    +2  Misses       1645     1645             Partials      356      356
FlagCoverage Δ
unittest87.15% <ø> (+<0.01%)⬆️

Flags with carried forward coverage won't be shown.Click here to find out more.

see 2 files with indirect coverage changes

@kirkwaiblingerkirkwaiblinger added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labelJan 20, 2025
@JoshuaKGoldbergJoshuaKGoldberg merged commit609a78f intotypescript-eslint:mainJan 20, 2025
64 of 66 checks passed
@ronamironami deleted the fix-no-unnecessary-type-assertion-template-literal-tests branchJanuary 20, 2025 15:15
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsJan 28, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@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.

3 participants
@ronami@kirkwaiblinger@JoshuaKGoldberg
[8]ページ先頭
©2009-2025 Movatter.jp