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(eslint-plugin): [no-deprecated] add rule#9783

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

JoshuaKGoldberg
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg commentedAug 12, 2024•
edited
Loading

PR Checklist

Overview

Reimplements logic from scratch - both for licensing reasons and because I wanted to see how the structure would differ from previous implementations. After doing it myself, then looking back at the others, I think it ended up closest to eslint-plugin-deprecation by coincidence.

💖

Zamiell, SchroederSteffen, gund, controversial, and alecmev reacted with thumbs up emojiZamiell, raftario, richardsimko, reduckted, controversial, AhmedBaset, boris-petrov, and karlhorky reacted with heart emojireduckted and karlhorky reacted with rocket emoji
@typescript-eslint
Copy link
Contributor

Thanks for the PR,@JoshuaKGoldberg!

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 commentedAug 12, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commit5cd8b82
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/66c5f66bce745d0008b76f6f
😎 Deploy Previewhttps://deploy-preview-9783--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 commentedAug 12, 2024•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit5cd8b82. 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 2 targets

Sent with 💌 fromNxCloud.

@JoshuaKGoldbergJoshuaKGoldberg changed the titleWIP: no-deprecationfeat(eslint-plugin): [no-deprecation] add ruleAug 15, 2024
@JoshuaKGoldbergJoshuaKGoldberg marked this pull request as ready for reviewAugust 15, 2024 17:15
@JoshuaKGoldbergJoshuaKGoldberg marked this pull request as draftAugust 15, 2024 17:19
eslint-visitor-keys: ^3.4.1
checksum: 67c7e6003d5af042d8703d11538fca9d76899f0119130b373402819ae43f0bc90d18656aa7add25a24427ccf1a0efd0804157ba83b0d4e145f06107d7d1b7433
languageName: node
linkType: hard
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Aside: it is very pleasing to no longer have a dependency on v6 versions of our packages. 🙃

kirkwaiblinger and armano2 reacted with heart emoji
@codecovCodecov
Copy link

codecovbot commentedAug 15, 2024•
edited
Loading

Codecov Report

Attention: Patch coverage is92.75362% with5 lines in your changes missing coverage. Please review.

Project coverage is 88.04%. Comparing base(6250dab) to head(5cd8b82).
Report is 1 commits behind head on main.

FilesPatch %Lines
packages/eslint-plugin/src/rules/no-deprecated.ts92.75%1 Missing and 4 partials⚠️
Additional details and impacted files
@@            Coverage Diff             @@##             main    #9783      +/-   ##==========================================+ Coverage   88.01%   88.04%   +0.02%==========================================  Files         406      407       +1       Lines       13879    13948      +69       Branches     4055     4078      +23     ==========================================+ Hits        12216    12280      +64- Misses       1354     1355       +1- Partials      309      313       +4
FlagCoverage Δ
unittest88.04% <92.75%> (+0.02%)⬆️

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

FilesCoverage Δ
packages/type-utils/src/typeFlagUtils.ts84.61% <ø> (ø)
packages/typescript-estree/src/convert.ts28.90% <ø> (ø)
packages/typescript-estree/src/getModifiers.ts66.66% <ø> (ø)
packages/typescript-estree/src/node-utils.ts51.34% <ø> (ø)
packages/utils/src/ts-eslint/RuleTester.ts50.00% <ø> (ø)
packages/eslint-plugin/src/rules/no-deprecated.ts92.75% <92.75%> (ø)

@JoshuaKGoldbergJoshuaKGoldberg marked this pull request as ready for reviewAugust 16, 2024 01:46
@JoshuaKGoldbergJoshuaKGoldberg requested a review froma teamAugust 16, 2024 01:47
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.

Looks great to me, just a few questions!

JoshuaKGoldbergand others added2 commitsAugust 19, 2024 07:37
Co-authored-by: Kirk Waiblinger <kirk.waiblinger@gmail.com>
kirkwaiblinger
kirkwaiblinger previously approved these changesAug 20, 2024
Josh-Cena
Josh-Cena previously approved these changesAug 20, 2024
Copy link
Member

@Josh-CenaJosh-Cena left a comment

Choose a reason for hiding this comment

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

Maybe mentiondeprecation/deprecation and why we have this rule?

JoshuaKGoldberg reacted with thumbs up emoji
kirkwaiblinger
kirkwaiblinger previously approved these changesAug 20, 2024
bradzacher
bradzacher previously approved these changesAug 21, 2024
symbol: ts.Signature | ts.Symbol | undefined,
): string | undefined {
const tag = symbol
?.getJsDocTags(checker)
Copy link
Member

Choose a reason for hiding this comment

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

this forces us to do "jsdoc parsing" by default :(
I kind of wish we could do it without using this -- eg with a regex so we can speed up parsing.

Copy link
MemberAuthor

@JoshuaKGoldbergJoshuaKGoldbergAug 21, 2024•
edited
Loading

Choose a reason for hiding this comment

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

OH, this just reminded me we haveJSDocParsingMode. AddingjsDocParsingMode: 'none' to the test file'slanguageOptions.parserOptions causes all theinvalid tests to fail for not having any parsed JSDocs. Great.

I think we're going to want to do our own regex-based parsing. I'll put the PR back in draft in the meantime.

bradzacher reacted with heart emoji
Copy link
Member

Choose a reason for hiding this comment

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

I think this is one of the few usecases of jsdoc via TS AST in the ecosystem. So yeah if we can remove this then we can consider flipping the default in the next version to speed up parsing in the general case!

JoshuaKGoldberg reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Well, I tried using onsymbol.valueDeclaration andsymbol.valueDeclaration.parent in the firstinvalid test case:

  • ts.getJSDocCommentsAndTags: Nothing retrieved
  • tsutils.forEachComment: Retrieved the comment, but took ~750ms

deprecation/deprecation also usessymbol.getJsDocTags and suffers from the same issue. We don't (yet?) defaultjsdocParsingMode to'none' or'type-info' for lint users. I added a thrown error for those modes in5cd8b82.

Back-linking to past conversation (cc@jakebailey as FYI):#7997 (comment)

Copy link
MemberAuthor

@JoshuaKGoldbergJoshuaKGoldbergAug 21, 2024•
edited
Loading

Choose a reason for hiding this comment

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

Proposal: let's treat"do our own regex-based parsing" as a followup?

Edit:#9857

Copy link
Member

Choose a reason for hiding this comment

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

Yeah for sure - file an issue.
Wasnt sposed to be a blocking comment. More of a "hey if we can that'd be good" thing

JoshuaKGoldberg reacted with thumbs up emoji
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps it's worth adding@deprecated to the list of tags that we parse out unconditionally in the API? Would that be simpler?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

That could work yeah! Do you know what the performance impact would be?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd have to test and see. Unfortunate though as my enum names were too clever,@deprecated is not used for checking :(

JoshuaKGoldberg reacted with laugh emoji
@JoshuaKGoldbergJoshuaKGoldberg marked this pull request as ready for reviewAugust 21, 2024 14:30
@JoshuaKGoldbergJoshuaKGoldberg removed the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labelAug 21, 2024
@FloEdelmann
Copy link
Contributor

FloEdelmann commentedAug 27, 2024•
edited
Loading

The rule was renamed tono-deprecated before merge, could you please adjust the PR title and the changelog, to reduce confusion?

JoshuaKGoldberg reacted with thumbs up emoji

@JoshuaKGoldbergJoshuaKGoldberg changed the titlefeat(eslint-plugin): [no-deprecation] add rulefeat(eslint-plugin): [no-deprecated] add ruleAug 27, 2024
@JoshuaKGoldberg
Copy link
MemberAuthor

Alas, changing the commit history & linked changelogs would be hard 😞 sorry about that. I edited the PR title just to help folks find it.

FloEdelmann reacted with thumbs up emoji

@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsSep 6, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@gundgundgund left review comments

@jakebaileyjakebaileyjakebailey left review comments

@bradzacherbradzacherbradzacher approved these changes

@kirkwaiblingerkirkwaiblingerkirkwaiblinger left review comments

@Josh-CenaJosh-CenaJosh-Cena left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rule proposal: Warn when deprecated APIs are used (eslint-plugin-deprecation)
7 participants
@JoshuaKGoldberg@FloEdelmann@gund@jakebailey@bradzacher@kirkwaiblinger@Josh-Cena
[8]ページ先頭
©2009-2025 Movatter.jp