Conversation
|
Merging this PR will not alter performance
Comparing Footnotes
|
WalkthroughReplaces CLI end-to-end ESLint migrator checks with fixture-driven snapshot tests and adds multiple JSONC fixtures under Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.claude/skills/eslint-migrate-options/SKILL.md (2)
294-327:⚠️ Potential issue | 🟠 MajorIncomplete review context: actual test infrastructure missing.
This PR is titled "test: infra for custom rule migrators" and the AI summary describes actual code changes including:
merge_biome_config_with_eslint()helper function- Test harness using
tests_macros::gen_tests!- JSONC test fixtures for various rules
tests_macrosdev-dependency additionHowever, only the skill documentation file is provided for review. Please include all changed files so the actual test infrastructure can be reviewed alongside the documentation updates.
Based on learnings, all code changes must include appropriate tests, and the test infrastructure itself should be reviewed for correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/eslint-migrate-options/SKILL.md around lines 294 - 327, The PR only updates the SKILL.md docs but references missing implementation and tests; include the actual changed source and test files so reviewers can validate the infrastructure: add the implementation of merge_biome_config_with_eslint(), the test harness using tests_macros::gen_tests! (and the tests_macros crate as a dev-dependency), the generated test file eslint_to_biome.rs, and the JSONC fixtures under crates/biome_cli/tests/specs/migrate_eslint/ (one fixture per case plus .snap files) so the migrator spec runner can be exercised; ensure Rules::deserialize arm, Rule::name(), From impls, and migrate_eslint_any_rule() usage are present in the added files so reviewers can verify behavior.
1-6:⚠️ Potential issue | 🟡 MinorImprove AI assistance disclosure format.
The PR description mentions "generated by gpt 5.4", but based on project learnings, AI assistance should be disclosed using a proper format such as:
> This PR was created with AI assistanceor
> This PR was created with AI assistance (ChatGPT/GPT-5.4)Please update the PR description to follow the standard disclosure format. Based on learnings, the project requires clear disclosure of AI assistance in pull requests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/eslint-migrate-options/SKILL.md around lines 1 - 6, The PR description currently contains the phrase "generated by gpt 5.4"; update the description text for the skill "eslint-migrate-options" to use the standard AI-assistance disclosure block instead (e.g., a single-line markdown blockquote like "> This PR was created with AI assistance" or "> This PR was created with AI assistance (ChatGPT/GPT-5.4)") so the disclosure follows project conventions and replaces the raw "generated by gpt 5.4" wording.
🧹 Nitpick comments (1)
crates/biome_cli/src/execute/migrate/eslint_to_biome.rs (1)
945-974: Spec-driven test harness looks good, but only supports legacy ESLint configs.The
MigratorSpecstruct (line 841) deserialiseseslintasLegacyConfigData, so flat ESLint configs aren't supported by this test harness. If you need to test flat config migrations in future, you'll need to extend the spec format.The double-deserialization (lines 954-957 for validation, then again inside
build_legacy_migration_snapshot) is intentional validation at two layers—fair enough.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_cli/src/execute/migrate/eslint_to_biome.rs` around lines 945 - 974, The spec-driven test only supports legacy ESLint configs because MigratorSpec currently deserializes the "eslint" field as LegacyConfigData; update MigratorSpec and deserialize_migrator_spec to accept either the existing LegacyConfigData or a new FlatConfigData (e.g., via an enum like EslintConfig::Legacy(LegacyConfigData) | Flat(FlatConfigData)), and then branch in the test harness so run_migrator_spec_test calls the correct snapshot builder (either keep build_legacy_migration_snapshot for legacy inputs and add a new build_flat_migration_snapshot or a unified build_migration_snapshot that handles both variants) so flat ESLint configs are supported by the spec tests; ensure you reference MigratorSpec, deserialize_migrator_spec, build_legacy_migration_snapshot, and run_migrator_spec_test when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.claude/skills/eslint-migrate-options/SKILL.md:
- Around line 294-327: The PR only updates the SKILL.md docs but references
missing implementation and tests; include the actual changed source and test
files so reviewers can validate the infrastructure: add the implementation of
merge_biome_config_with_eslint(), the test harness using
tests_macros::gen_tests! (and the tests_macros crate as a dev-dependency), the
generated test file eslint_to_biome.rs, and the JSONC fixtures under
crates/biome_cli/tests/specs/migrate_eslint/ (one fixture per case plus .snap
files) so the migrator spec runner can be exercised; ensure Rules::deserialize
arm, Rule::name(), From impls, and migrate_eslint_any_rule() usage are present
in the added files so reviewers can verify behavior.
- Around line 1-6: The PR description currently contains the phrase "generated
by gpt 5.4"; update the description text for the skill "eslint-migrate-options"
to use the standard AI-assistance disclosure block instead (e.g., a single-line
markdown blockquote like "> This PR was created with AI assistance" or "> This
PR was created with AI assistance (ChatGPT/GPT-5.4)") so the disclosure follows
project conventions and replaces the raw "generated by gpt 5.4" wording.
---
Nitpick comments:
In `@crates/biome_cli/src/execute/migrate/eslint_to_biome.rs`:
- Around line 945-974: The spec-driven test only supports legacy ESLint configs
because MigratorSpec currently deserializes the "eslint" field as
LegacyConfigData; update MigratorSpec and deserialize_migrator_spec to accept
either the existing LegacyConfigData or a new FlatConfigData (e.g., via an enum
like EslintConfig::Legacy(LegacyConfigData) | Flat(FlatConfigData)), and then
branch in the test harness so run_migrator_spec_test calls the correct snapshot
builder (either keep build_legacy_migration_snapshot for legacy inputs and add a
new build_flat_migration_snapshot or a unified build_migration_snapshot that
handles both variants) so flat ESLint configs are supported by the spec tests;
ensure you reference MigratorSpec, deserialize_migrator_spec,
build_legacy_migration_snapshot, and run_migrator_spec_test when making the
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2e7941c9-a0c8-4f53-b84a-99c33d4c0dc9
⛔ Files ignored due to path filters (12)
Cargo.lockis excluded by!**/*.lockand included by**crates/biome_cli/tests/specs/migrate_eslint/jestConsistentTestIt/defaults.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/jestConsistentTestIt/withOptions.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/jsxA11yAriaRole/basic.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/noRestrictedGlobals/basic.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/typescriptArrayType/generic.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/typescriptConsistentTypeImports/inlineTypeImports.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/typescriptExplicitMemberAccessibility/explicit.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/typescriptNamingConvention/basic.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/unicornFilenameCase/basic.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/vitestConsistentTestIt/withOptions.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_configuration/src/generated/linter_options_check.rsis excluded by!**/generated/**,!**/generated/**and included by**
📒 Files selected for processing (14)
.claude/skills/eslint-migrate-options/SKILL.mdcrates/biome_cli/Cargo.tomlcrates/biome_cli/src/execute/migrate.rscrates/biome_cli/src/execute/migrate/eslint_to_biome.rscrates/biome_cli/tests/specs/migrate_eslint/jestConsistentTestIt/defaults.jsonccrates/biome_cli/tests/specs/migrate_eslint/jestConsistentTestIt/withOptions.jsonccrates/biome_cli/tests/specs/migrate_eslint/jsxA11yAriaRole/basic.jsonccrates/biome_cli/tests/specs/migrate_eslint/noRestrictedGlobals/basic.jsonccrates/biome_cli/tests/specs/migrate_eslint/typescriptArrayType/generic.jsonccrates/biome_cli/tests/specs/migrate_eslint/typescriptConsistentTypeImports/inlineTypeImports.jsonccrates/biome_cli/tests/specs/migrate_eslint/typescriptExplicitMemberAccessibility/explicit.jsonccrates/biome_cli/tests/specs/migrate_eslint/typescriptNamingConvention/basic.jsonccrates/biome_cli/tests/specs/migrate_eslint/unicornFilenameCase/basic.jsonccrates/biome_cli/tests/specs/migrate_eslint/vitestConsistentTestIt/withOptions.jsonc
01bc9c1 to
2c4bf81
Compare
2c4bf81 to
4bb602d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_cli/src/execute/migrate/eslint_to_biome.rs (1)
838-844:MigratorSpeconly supports legacy ESLint configs.The
eslint_configfield is typed asLegacyConfigData, so flat ESLint configs (eslint.config.jsstyle) cannot be tested with this infrastructure. If migrating flat configs is in scope, consider using an enum wrapper or adding a separate spec type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_cli/src/execute/migrate/eslint_to_biome.rs` around lines 838 - 844, MigratorSpec currently types eslint_config as LegacyConfigData which excludes flat ESLint configs (e.g., eslint.config.js); update the spec to accept both legacy and flat formats by replacing the single LegacyConfigData field with a discriminated enum or union wrapper (e.g., an enum EslintConfig { Legacy(LegacyConfigData), Flat(FlatConfigData) } or add an additional field) and adjust deserialization directives on the enum/fields; modify references to MigratorSpec::eslint_config handling code to pattern-match the new enum variants so both legacy and flat configs are supported in the migration tests and runtime (identify MigratorSpec and eslint_config and LegacyConfigData to locate changes).
🤖 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_cli/src/execute/migrate/eslint_to_biome.rs`:
- Around line 952-953: The code currently calls
serde_json::from_str(&input_code) which will fail on JSONC (comments/trailing
commas); instead call the existing format_json helper to normalize JSONC first
and then deserialize — i.e. compute let formatted = format_json(&input_code) and
then call serde_json::from_str(&formatted). Update the site where raw_spec is
created (the let raw_spec: serde_json::Value = ... line) to use formatted input
so format_json, input_code, and serde_json::from_str are used as described.
---
Nitpick comments:
In `@crates/biome_cli/src/execute/migrate/eslint_to_biome.rs`:
- Around line 838-844: MigratorSpec currently types eslint_config as
LegacyConfigData which excludes flat ESLint configs (e.g., eslint.config.js);
update the spec to accept both legacy and flat formats by replacing the single
LegacyConfigData field with a discriminated enum or union wrapper (e.g., an enum
EslintConfig { Legacy(LegacyConfigData), Flat(FlatConfigData) } or add an
additional field) and adjust deserialization directives on the enum/fields;
modify references to MigratorSpec::eslint_config handling code to pattern-match
the new enum variants so both legacy and flat configs are supported in the
migration tests and runtime (identify MigratorSpec and eslint_config and
LegacyConfigData to locate changes).
🪄 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: d214f8dc-019c-4bcc-bc2c-5a922aa5f8b4
⛔ Files ignored due to path filters (11)
Cargo.lockis excluded by!**/*.lockand included by**crates/biome_cli/tests/specs/migrate_eslint/jestConsistentTestIt/defaults.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/jestConsistentTestIt/withOptions.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/jsxA11yAriaRole/basic.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/noRestrictedGlobals/basic.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/typescriptArrayType/generic.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/typescriptConsistentTypeImports/inlineTypeImports.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/typescriptExplicitMemberAccessibility/explicit.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/typescriptNamingConvention/basic.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/unicornFilenameCase/basic.jsonc.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/specs/migrate_eslint/vitestConsistentTestIt/withOptions.jsonc.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (15)
.claude/skills/eslint-migrate-options/SKILL.mdcrates/biome_analyze/CONTRIBUTING.mdcrates/biome_cli/Cargo.tomlcrates/biome_cli/src/execute/migrate.rscrates/biome_cli/src/execute/migrate/eslint_to_biome.rscrates/biome_cli/tests/specs/migrate_eslint/jestConsistentTestIt/defaults.jsonccrates/biome_cli/tests/specs/migrate_eslint/jestConsistentTestIt/withOptions.jsonccrates/biome_cli/tests/specs/migrate_eslint/jsxA11yAriaRole/basic.jsonccrates/biome_cli/tests/specs/migrate_eslint/noRestrictedGlobals/basic.jsonccrates/biome_cli/tests/specs/migrate_eslint/typescriptArrayType/generic.jsonccrates/biome_cli/tests/specs/migrate_eslint/typescriptConsistentTypeImports/inlineTypeImports.jsonccrates/biome_cli/tests/specs/migrate_eslint/typescriptExplicitMemberAccessibility/explicit.jsonccrates/biome_cli/tests/specs/migrate_eslint/typescriptNamingConvention/basic.jsonccrates/biome_cli/tests/specs/migrate_eslint/unicornFilenameCase/basic.jsonccrates/biome_cli/tests/specs/migrate_eslint/vitestConsistentTestIt/withOptions.jsonc
✅ Files skipped from review due to trivial changes (11)
- crates/biome_cli/tests/specs/migrate_eslint/jestConsistentTestIt/withOptions.jsonc
- crates/biome_cli/tests/specs/migrate_eslint/jestConsistentTestIt/defaults.jsonc
- crates/biome_cli/tests/specs/migrate_eslint/unicornFilenameCase/basic.jsonc
- crates/biome_cli/tests/specs/migrate_eslint/noRestrictedGlobals/basic.jsonc
- crates/biome_cli/tests/specs/migrate_eslint/typescriptArrayType/generic.jsonc
- crates/biome_cli/tests/specs/migrate_eslint/vitestConsistentTestIt/withOptions.jsonc
- crates/biome_cli/tests/specs/migrate_eslint/typescriptConsistentTypeImports/inlineTypeImports.jsonc
- crates/biome_cli/tests/specs/migrate_eslint/jsxA11yAriaRole/basic.jsonc
- crates/biome_cli/tests/specs/migrate_eslint/typescriptExplicitMemberAccessibility/explicit.jsonc
- crates/biome_analyze/CONTRIBUTING.md
- crates/biome_cli/tests/specs/migrate_eslint/typescriptNamingConvention/basic.jsonc
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/biome_cli/src/execute/migrate.rs
- .claude/skills/eslint-migrate-options/SKILL.md
- crates/biome_cli/Cargo.toml
| let raw_spec: serde_json::Value = | ||
| serde_json::from_str(&input_code).expect("migrator spec should be valid JSON"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if serde_json::Value implements Deserializable from biome_deserialize
ast-grep --pattern 'impl$$$Deserializable$$$for$$$Value$$$'Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
cat -n crates/biome_cli/src/execute/migrate/eslint_to_biome.rs | sed -n '940,975p'Repository: biomejs/biome
Length of output: 1757
🏁 Script executed:
# Check what functions are available in biome_deserialize for JSON parsing
rg 'pub.*fn.*deserialize.*json' crates/biome_deserialize/src/ -A 3Repository: biomejs/biome
Length of output: 599
🏁 Script executed:
# Check how JSONC is parsed elsewhere in the biome codebase
rg 'allow_comments|allow_trailing_commas' --type rust -B 2 -A 2 | head -80Repository: biomejs/biome
Length of output: 5767
🏁 Script executed:
rg 'fn deserialize_migrator_spec' -A 20 crates/biome_cli/src/execute/migrate/Repository: biomejs/biome
Length of output: 2014
🏁 Script executed:
# Search for JSONC parsing that results in serde_json::Value
rg 'serde_json::Value|serde_json::from_str' crates/biome_cli/src/execute/migrate/ -B 3 -A 3Repository: biomejs/biome
Length of output: 918
🏁 Script executed:
# Check if there's an existing pattern for parsing JSONC into serde_json::Value
rg 'biome_json_parser.*serde_json|serde_json.*biome_json_parser' --type rust -B 2 -A 5 | head -60Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
find crates/biome_cli/tests/specs/migrate_eslint -name '*.jsonc' -o -name '*.json' | head -5Repository: biomejs/biome
Length of output: 445
🏁 Script executed:
# Check if any test files contain comments or trailing commas
fd '\.jsonc$' crates/biome_cli/tests/specs/migrate_eslint -x grep -l '//' {} \;Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Look at an example test file to understand the structure
find crates/biome_cli/tests/specs/migrate_eslint -type f \( -name '*.json' -o -name '*.jsonc' \) | head -1 | xargs head -30Repository: biomejs/biome
Length of output: 155
🏁 Script executed:
rg 'parse_json|JsonRoot' crates/biome_cli/src/execute/migrate/eslint_to_biome.rs -B 2 -A 10 | head -80Repository: biomejs/biome
Length of output: 1136
🏁 Script executed:
# Check how serde_json values are created from parsed JSON elsewhere
rg 'serde_json::from_value|serde_json::to_value|serde_json::json!' crates/biome_cli/src/ -B 2 -A 2 | head -60Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Look at the complete format_json function and nearby utilities
rg 'fn format_json|fn parse_json' crates/biome_cli/src/execute/migrate/eslint_to_biome.rs -A 20Repository: biomejs/biome
Length of output: 874
serde_json doesn't support JSONC; use the existing format_json helper.
The glob pattern at line 945 includes *.jsonc files, but serde_json::from_str will panic if any fixture contains comments or trailing commas. Although current test files don't use JSONC features, this is a latent issue.
The suggested fix won't compile—serde_json::Value doesn't implement the Deserializable trait. Instead, reuse the existing format_json function (which already handles JSONC parsing and normalisation):
let formatted = format_json(&input_code);
let raw_spec: serde_json::Value =
serde_json::from_str(&formatted).expect("migrator spec should be valid JSON");This normalises JSONC to valid JSON before deserialising, aligning with how the helper is used elsewhere in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_cli/src/execute/migrate/eslint_to_biome.rs` around lines 952 -
953, The code currently calls serde_json::from_str(&input_code) which will fail
on JSONC (comments/trailing commas); instead call the existing format_json
helper to normalize JSONC first and then deserialize — i.e. compute let
formatted = format_json(&input_code) and then call
serde_json::from_str(&formatted). Update the site where raw_spec is created (the
let raw_spec: serde_json::Value = ... line) to use formatted input so
format_json, input_code, and serde_json::from_str are used as described.
Summary
Having a bunch of CLI tests for custom migrators isn't great. This aims to resolve that.
generated by gpt 5.4
Test Plan
Docs