-
-
Notifications
You must be signed in to change notification settings - Fork 781
feat(js_analyze): implement useFind #8100
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
Conversation
🦋 Changeset detectedLatest commit: af1a98a The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ebc813c to
6e4ce5f
Compare
WalkthroughAdds a new nursery lint rule Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
6e4ce5f to
3d75efe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/use_find.rs (1)
106-119: Minor: Clarify diagnostic message.The diagnostic message on Line 112 says
"Array#filter[0]", which might be confused with computed member access. Consider"Array#filter()[0]"to be clearer.Apply this diff:
markup! { - "Prefer using Array#find() over Array#filter[0]." + "Prefer using Array#find() over Array#filter()[0]." },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rsis excluded by!**/migrate/eslint_any_rule_to_biome.rsand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_configuration/src/generated/domain_selector.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/src/lint/nursery.rsis excluded by!**/nursery.rsand included by**crates/biome_js_analyze/tests/specs/nursery/useFind/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useFind/valid.ts.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (7)
.changeset/all-kiwis-poke.md(1 hunks)crates/biome_js_analyze/src/lint/nursery/use_find.rs(1 hunks)crates/biome_js_analyze/tests/specs/nursery/useFind/invalid.ts(1 hunks)crates/biome_js_analyze/tests/specs/nursery/useFind/valid.ts(1 hunks)crates/biome_js_type_info/src/type.rs(1 hunks)crates/biome_rule_options/src/lib.rs(1 hunks)crates/biome_rule_options/src/use_find.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/lint/nursery/use_find.rs (1)
crates/biome_analyze/src/rule.rs (4)
recommended(602-605)sources(617-620)same(246-251)domains(632-635)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Validate rules documentation
- GitHub Check: Documentation
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: autofix
- GitHub Check: Check Dependencies
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Test Node.js API
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Check JS Files
- GitHub Check: Bench (biome_js_formatter)
🔇 Additional comments (9)
crates/biome_rule_options/src/use_find.rs (1)
1-6: LGTM!Empty options struct is appropriate for a rule with no configurable parameters. Standard derives and serde attributes are correctly applied.
.changeset/all-kiwis-poke.md (1)
1-13: LGTM!Changeset documentation is clear and follows the standard format with helpful examples.
crates/biome_js_analyze/tests/specs/nursery/useFind/invalid.ts (1)
1-8: LGTM!Test cases comprehensively cover the invalid patterns, including chaining scenarios and optional chaining.
crates/biome_rule_options/src/lib.rs (1)
295-295: LGTM!Module export is correctly placed in alphabetical order.
crates/biome_js_analyze/tests/specs/nursery/useFind/valid.ts (1)
1-13: LGTM!Valid test cases properly cover edge cases, including non-zero indices, method chaining, and object methods.
crates/biome_js_analyze/src/lint/nursery/use_find.rs (3)
1-46: LGTM!Rule declaration and metadata are well-structured. Documentation includes clear examples, proper attribution to ESLint TypeScript's
prefer-find, and appropriate configuration for a nursery rule.
48-51: LGTM!Helper function correctly identifies first position using both number and bigint literal checks.
59-104: LGTM with a note on type safety.The implementation correctly detects
.filter()[0]and.filter().at(0)patterns. However, the rule relies on method name matching without verifying thatfilteris actually called on an Array type. This could theoretically flag custom objects with afiltermethod, though in practice this is likely acceptable for a nursery rule.If stricter type checking is desired, consider verifying that the callee object is an array type using the type inference system.
crates/biome_js_type_info/src/type.rs (1)
203-216: The duplicate method definition claim is incorrect.Verification shows only one
is_bigint_literaldefinition at line 204. There is no duplication in the codebase. The original review comment was based on a false premise.Likely an incorrect or invalid review comment.
CodSpeed Performance ReportMerging #8100 will not alter performanceComparing Summary
Footnotes
|
dyc3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we provide an unsafe code fix? No strong feelings on if you do it in this PR or a new one.
Would do that in a new PR |
Summary
Implement Eslint's
prefer-findCloses #7655
Test Plan
Docs