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

feat(linter): implement noRestrictedTypes #3585

Merged
merged 7 commits into from
Sep 3, 2024

Conversation

minht11
Copy link
Contributor

@minht11 minht11 commented Aug 4, 2024

Summary

Closes #3583 Rule allows user to specify types which they want to prevent used in their code. Similar to noRestrictedGlobals and noRestrictedImports

Test Plan

Added snapshots

@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project A-Linter Area: linter L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis labels Aug 4, 2024
Copy link

codspeed-hq bot commented Aug 4, 2024

CodSpeed Performance Report

Merging #3585 will degrade performances by 6.22%

Comparing minht11:feat/no-banned-types-options (efaad79) with main (73656ec)

Summary

❌ 1 regressions
✅ 98 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main minht11:feat/no-banned-types-options Change
pure_9395922602181450299.css[cached] 3.5 ms 3.7 ms -6.22%

@minht11 minht11 force-pushed the feat/no-banned-types-options branch from 20bf1dd to 30cbbd1 Compare August 4, 2024 22:36
@github-actions github-actions bot added the A-Changelog Area: changelog label Aug 4, 2024
@dyc3
Copy link
Contributor

dyc3 commented Aug 4, 2024

Why can't we just add user options to the old rule with defaults that keep the behavior the same? Is a new rule really necessary?

@minht11
Copy link
Contributor Author

minht11 commented Aug 5, 2024

I think the idea is that we need to rename the rule to be more consistent with other rules noRestrictedImports, noRestrictedGlobals and now noBannedTypes noRestrictedTypes, ts-eslint also renamed theirs. Adding the new rule allows users to migrate incrementally while we deprecate old one.

Copy link
Member

@Conaclos Conaclos left a comment

Choose a reason for hiding this comment

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

I think that we should follow the lead of TypeScript ESLint that decided to split no-banned-types into several rules. For now, we could keep noBannedTypes to ban types like Boolean, and we could dedicate noReswtrictedTypes to user-defined types. This will simplify the rule impl and make it more consistent with other noRstricted* rules.

Comment on lines 262 to 263
message: String,
fix_with: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

We could make the message optional and generate a default message such as Use <fix_with> type instead.
Also, I could rename fix_with with use.

Suggested change
message: String,
fix_with: Option<String>,
message: Option<String>,
use: Option<String>,

We could add a validation state that ensures that use is a valid identifier (a function js_ident allows checking it) and at least one field is specified (either message, or use, or both).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the field, not sure about making message field optional, banning types should be very intentional action and default explanation would not be helpful for other people.

I would like to make option to accept string directly like this

type Options = {
  foo: string | { message: string }
}

Though I am not sure how to do that with Serde

@ematipico
Copy link
Member

@minht11 do you need any help to land this PR?

@minht11
Copy link
Contributor Author

minht11 commented Aug 27, 2024

@ematipico Sorry been bit distracted, will fix up this PR this week.

@minht11
Copy link
Contributor Author

minht11 commented Sep 1, 2024

I think that we should follow the lead of TypeScript ESLint that decided to split no-banned-types into several rules. For now, we could keep noBannedTypes to ban types like Boolean, and we could dedicate noReswtrictedTypes to user-defined types. This will simplify the rule impl and make it more consistent with other noRstricted* rules.

It does make sense. Looking into typescript-eslint 8 docs now I see they made this rule only for custom types, when I originally implemented this rule I was referencing ts-eslint 7.

I will remove default banned types and leave that functionality for noBannedTypes instead. It is consistent with noRestrictedGlobals and noRestrictedImports

@minht11 minht11 marked this pull request as draft September 1, 2024 23:00
@minht11 minht11 marked this pull request as ready for review September 2, 2024 21:38
Copy link
Member

@Conaclos Conaclos 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. I left last suggestions. Once addressed we can merge 🚀

@minht11 minht11 merged commit 70557f2 into biomejs:main Sep 3, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-CLI Area: CLI A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Project Area: project L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📎 implement noRestrictedTypes
4 participants