Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdded a new Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_analyze/src/rule.rs`:
- Around line 199-200: Add unit tests that exercise the new RuleSource::Sherif
branch: instantiate rules using RuleSource::Sherif("repo/path") and assert
correct behaviour for Display (string formatting via impl Display),
ordering/comparison (PartialOrd/Ord or Rule::cmp), naming (Rule::name or similar
accessor), namespace generation (Rule::namespace) and URL generation (Rule::url
or to_url method). Put tests in the existing rule tests module (or create a new
#[cfg(test)] mod) and include multiple cases (different sherif identifiers and
edge cases) that assert expected strings and ordering to cover the branches
added for Sherif.
- Around line 199-200: The enum variant Sherif(&'a str) is a non-ESLint source
but the is_eslint_plugin method still treats it as plugin-like; update
is_eslint_plugin to explicitly exclude Self::Sherif(_) from returning true (add
a match arm or conditional so Sherif(_) returns false) so Sherif is not
classified as an ESLint plugin, referencing the enum variant Sherif and the
method is_eslint_plugin to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1edbc9a6-b423-4ec2-8b94-f463e295158f
📒 Files selected for processing (2)
crates/biome_analyze/src/rule.rscrates/biome_json_analyze/src/assist/source/use_sorted_package_json.rs
| /// Rules from [Sherif](https://github.com/QuiiBz/sherif) | ||
| Sherif(&'a str), |
There was a problem hiding this comment.
Please add targeted tests for the new RuleSource::Sherif branches.
These lines add new behaviour across display, ordering, naming, namespace, and URL generation, but there’s no accompanying test coverage in this PR. A couple of focused unit tests here would save future us from spelunking regressions at 2am.
As per coding guidelines **/*.{rs,ts,tsx,js}: All code changes MUST include appropriate tests: ... and bug fixes require tests that reproduce and validate the fix.
Also applies to: 259-259, 329-329, 396-397, 409-410, 516-516
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_analyze/src/rule.rs` around lines 199 - 200, Add unit tests that
exercise the new RuleSource::Sherif branch: instantiate rules using
RuleSource::Sherif("repo/path") and assert correct behaviour for Display (string
formatting via impl Display), ordering/comparison (PartialOrd/Ord or Rule::cmp),
naming (Rule::name or similar accessor), namespace generation (Rule::namespace)
and URL generation (Rule::url or to_url method). Put tests in the existing rule
tests module (or create a new #[cfg(test)] mod) and include multiple cases
(different sherif identifiers and edge cases) that assert expected strings and
ordering to cover the branches added for Sherif.
Sherif is currently misclassified as an ESLint plugin.
Line 199 adds a non-ESLint source, but is_eslint_plugin still treats it as plugin-like by default. Please exclude Self::Sherif(_) there.
Suggested fix
pub const fn is_eslint_plugin(&self) -> bool {
!matches!(
self,
Self::Clippy(_)
| Self::DenoLint(_)
| Self::Eslint(_)
| Self::GraphqlSchemaLinter(_)
| Self::Stylelint(_)
| Self::SortPackageJson
+ | Self::Sherif(_)
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Rules from [Sherif](https://github.com/QuiiBz/sherif) | |
| Sherif(&'a str), | |
| pub const fn is_eslint_plugin(&self) -> bool { | |
| !matches!( | |
| self, | |
| Self::Clippy(_) | |
| | Self::DenoLint(_) | |
| | Self::Eslint(_) | |
| | Self::GraphqlSchemaLinter(_) | |
| | Self::Stylelint(_) | |
| | Self::SortPackageJson | |
| | Self::Sherif(_) | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_analyze/src/rule.rs` around lines 199 - 200, The enum variant
Sherif(&'a str) is a non-ESLint source but the is_eslint_plugin method still
treats it as plugin-like; update is_eslint_plugin to explicitly exclude
Self::Sherif(_) from returning true (add a match arm or conditional so Sherif(_)
returns false) so Sherif is not classified as an ESLint plugin, referencing the
enum variant Sherif and the method is_eslint_plugin to locate the change.
Merging this PR will not alter performance
Comparing Footnotes
|
The record is currently 5: https://biomejs.dev/linter/rules/no-script-url/ |
Summary
Extra sources for the upcoming action useSortedPackageJson. Rule has not been released yet, so I don't think a changeset is necessary
Test Plan
Docs