feat(json_analyze): implement noTopLevelLiterals#9367
Conversation
🦋 Changeset detectedLatest commit: 743eb27 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 |
WalkthroughAdds a new nursery JSON lint rule 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.
🧹 Nitpick comments (1)
crates/biome_json_analyze/src/lint/nursery/use_top_level_interop.rs (1)
85-99: Consider using the value's range for more precise diagnostic highlighting.Currently
ctx.query().range()returns the range of the entireJsonRoot. For a more focused diagnostic, using the actual value's range would pinpoint the offending primitive rather than the whole document.♻️ Proposed refinement
fn diagnostic(ctx: &RuleContext<Self>, _state: &Self::State) -> Option<RuleDiagnostic> { - let span = ctx.query().range(); + let node = ctx.query(); + let value = node.value().ok()?; + let span = value.range(); Some( RuleDiagnostic::new(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_json_analyze/src/lint/nursery/use_top_level_interop.rs` around lines 85 - 99, The diagnostic currently highlights the whole JsonRoot by using ctx.query().range(); change it to use the inner value's range for precise highlighting (e.g., obtain the value via ctx.query().value() and use its range) and fall back to ctx.query().range() if the value is missing; update the span used in RuleDiagnostic::new inside the diagnostic function so the note and diagnostic target the offending primitive rather than the entire document.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_json_analyze/src/lint/nursery/use_top_level_interop.rs`:
- Around line 85-99: The diagnostic currently highlights the whole JsonRoot by
using ctx.query().range(); change it to use the inner value's range for precise
highlighting (e.g., obtain the value via ctx.query().value() and use its range)
and fall back to ctx.query().range() if the value is missing; update the span
used in RuleDiagnostic::new inside the diagnostic function so the note and
diagnostic target the offending primitive rather than the entire document.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b21a88b1-554c-4e81-a905-c3958463a0c4
⛔ Files ignored due to path filters (13)
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_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_json_analyze/tests/specs/nursery/useTopLevelInterop/invalid/boolean.json.snapis excluded by!**/*.snapand included by**crates/biome_json_analyze/tests/specs/nursery/useTopLevelInterop/invalid/null.json.snapis excluded by!**/*.snapand included by**crates/biome_json_analyze/tests/specs/nursery/useTopLevelInterop/invalid/number.json.snapis excluded by!**/*.snapand included by**crates/biome_json_analyze/tests/specs/nursery/useTopLevelInterop/invalid/string.json.snapis excluded by!**/*.snapand included by**crates/biome_json_analyze/tests/specs/nursery/useTopLevelInterop/valid/array.json.snapis excluded by!**/*.snapand included by**crates/biome_json_analyze/tests/specs/nursery/useTopLevelInterop/valid/empty-array.json.snapis excluded by!**/*.snapand included by**crates/biome_json_analyze/tests/specs/nursery/useTopLevelInterop/valid/empty-object.json.snapis excluded by!**/*.snapand included by**crates/biome_json_analyze/tests/specs/nursery/useTopLevelInterop/valid/object.json.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 (12)
.changeset/sour-beds-juggle.mdcrates/biome_json_analyze/src/lint/nursery/use_top_level_interop.rscrates/biome_json_analyze/tests/specs/nursery/useTopLevelInterop/invalid/boolean.jsoncrates/biome_json_analyze/tests/specs/nursery/useTopLevelInterop/invalid/null.jsoncrates/biome_json_analyze/tests/specs/nursery/useTopLevelInterop/invalid/number.jsoncrates/biome_json_analyze/tests/specs/nursery/useTopLevelInterop/invalid/string.jsoncrates/biome_json_analyze/tests/specs/nursery/useTopLevelInterop/valid/array.jsoncrates/biome_json_analyze/tests/specs/nursery/useTopLevelInterop/valid/empty-array.jsoncrates/biome_json_analyze/tests/specs/nursery/useTopLevelInterop/valid/empty-object.jsoncrates/biome_json_analyze/tests/specs/nursery/useTopLevelInterop/valid/object.jsoncrates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/use_top_level_interop.rs
Merging this PR will not alter performance
Comparing Footnotes
|
I like it!! |
|
Actually, this rule should be a |
noTopLevelLiteral? |
| fn diagnostic(ctx: &RuleContext<Self>, _state: &Self::State) -> Option<RuleDiagnostic> { | ||
| let span = ctx.query().range(); | ||
| Some( | ||
| RuleDiagnostic::new( | ||
| rule_category!(), | ||
| span, | ||
| markup! { | ||
| "Expected the top-level value to be an array or object." | ||
| }, | ||
| ) | ||
| .note(markup! { | ||
| "To ensure maximal interoperability with the oldest JSON parsers, make sure the top-level value is an array or object." | ||
| }), | ||
| ) | ||
| } |
There was a problem hiding this comment.
the diagnostic could be improved. rule pillars.
596c68f to
d00c1df
Compare
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 @.changeset/sour-beds-juggle.md:
- Line 5: The release note line is a fragment; update the sentence about the new
rule `noTopLevelLiterals` to be a full, clear sentence—e.g., "Added the nursery
rule `noTopLevelLiterals`, which requires the root-level value to be an array or
object." Locate the line referencing `noTopLevelLiterals` and replace the
fragment with this polished wording.
In `@crates/biome_rule_options/src/no_top_level_literals.rs`:
- Line 6: Add inline rustdoc for the NoTopLevelLiteralsOptions struct: above the
declaration of NoTopLevelLiteralsOptions document what the options control
(e.g., toggle the rule behavior or note there are currently no configurable
fields), the intended usage, and any default behavior; use Rust doc comments
(///) so generated docs are complete and follow the guideline for rules/options
documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f3fcc60-dc79-4cc2-a4a6-a31e20fc5abb
⛔ Files ignored due to path filters (13)
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_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/invalid/boolean.json.snapis excluded by!**/*.snapand included by**crates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/invalid/null.json.snapis excluded by!**/*.snapand included by**crates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/invalid/number.json.snapis excluded by!**/*.snapand included by**crates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/invalid/string.json.snapis excluded by!**/*.snapand included by**crates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/valid/array.json.snapis excluded by!**/*.snapand included by**crates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/valid/empty-array.json.snapis excluded by!**/*.snapand included by**crates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/valid/empty-object.json.snapis excluded by!**/*.snapand included by**crates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/valid/object.json.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 (12)
.changeset/sour-beds-juggle.mdcrates/biome_json_analyze/src/lint/nursery/no_top_level_literals.rscrates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/invalid/boolean.jsoncrates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/invalid/null.jsoncrates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/invalid/number.jsoncrates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/invalid/string.jsoncrates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/valid/array.jsoncrates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/valid/empty-array.jsoncrates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/valid/empty-object.jsoncrates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/valid/object.jsoncrates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/no_top_level_literals.rs
✅ Files skipped from review due to trivial changes (4)
- crates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/invalid/string.json
- crates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/valid/object.json
- crates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/valid/empty-array.json
- crates/biome_json_analyze/tests/specs/nursery/noTopLevelLiterals/invalid/number.json
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_rule_options/src/lib.rs
| #[derive(Default, Clone, Debug, Deserialize, Deserializable, Merge, Eq, PartialEq, Serialize)] | ||
| #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] | ||
| #[serde(rename_all = "camelCase", deny_unknown_fields, default)] | ||
| pub struct NoTopLevelLiteralsOptions {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add rustdoc for the options type.
NoTopLevelLiteralsOptions should have inline rustdoc so generated docs stay complete and consistent.
Suggested patch
+/// Options for the `noTopLevelLiterals` rule.
pub struct NoTopLevelLiteralsOptions {}As per coding guidelines, "**/*.rs: Use inline rustdoc documentation for rules, assists, and their options".
📝 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.
| pub struct NoTopLevelLiteralsOptions {} | |
| /// Options for the `noTopLevelLiterals` rule. | |
| pub struct NoTopLevelLiteralsOptions {} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_rule_options/src/no_top_level_literals.rs` at line 6, Add inline
rustdoc for the NoTopLevelLiteralsOptions struct: above the declaration of
NoTopLevelLiteralsOptions document what the options control (e.g., toggle the
rule behavior or note there are currently no configurable fields), the intended
usage, and any default behavior; use Rust doc comments (///) so generated docs
are complete and follow the guideline for rules/options documentation.
dyc3
left a comment
There was a problem hiding this comment.
Looks good. Address the bot's feedback on the changeset
Summary
Implement Eslint JSON's
top-level-interop, requiring the top-level value to be an array or object.Unsure about the naming of the rule, open for new names (perhaps
useTopLevelArrayOrObject?)Test Plan
unit tests
Docs