refactor: move tailwind sorter to its own crate#9348
Conversation
|
WalkthroughThis PR extracts Tailwind CSS class sorting logic from 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: 1
🤖 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_js_analyze/src/lint/nursery/use_sorted_classes/sort.rs`:
- Around line 108-133: get_sort_class_name_range currently allows zero-width
ranges (end == start) to be returned; change the empty-range guard to reject
zero-length ranges by returning None when end <= start instead of only when end
< start so it matches the Tailwind sorter behavior; update the condition in
get_sort_class_name_range (the end/start check before Some(TextRange::new(start,
end))) to use <= and keep the rest of the logic (offset_prefix/offset_suffix and
TextRange::new) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fb4ffe8e-5e48-4ffb-9dd1-97d6b0c48041
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lockand included by**
📒 Files selected for processing (12)
crates/biome_js_analyze/Cargo.tomlcrates/biome_js_analyze/benches/use_sorted_classes_parser.rscrates/biome_js_analyze/src/lint/nursery/use_sorted_classes.rscrates/biome_js_analyze/src/lint/nursery/use_sorted_classes/sort.rscrates/biome_tailwind_sort/Cargo.tomlcrates/biome_tailwind_sort/src/class_info.rscrates/biome_tailwind_sort/src/class_lexer.rscrates/biome_tailwind_sort/src/lib.rscrates/biome_tailwind_sort/src/presets.rscrates/biome_tailwind_sort/src/sort.rscrates/biome_tailwind_sort/src/sort_config.rscrates/biome_tailwind_sort/src/tailwind_preset.rs
| pub(crate) fn get_sort_class_name_range( | ||
| class_name: &TokenText, | ||
| range: &TextRange, | ||
| template_ctx: &Option<TemplateLiteralSpaceContext>, | ||
| ) -> Option<TextRange> { | ||
| let sort_context = template_ctx.as_ref().map(|ctx| ctx.to_sort_context()); | ||
|
|
||
| let mut class_iter = class_name.split_whitespace(); | ||
| let first_class_len = class_iter.next().map_or(0, |s| s.len()) as u32; | ||
| let last_class_len = class_iter.next_back().map_or(0, |s| s.len()) as u32; | ||
|
|
||
| let (ignore_prefix, ignore_suffix) = sort_context | ||
| .as_ref() | ||
| .map_or((false, false), |ctx| (ctx.ignore_prefix, ctx.ignore_suffix)); | ||
| let offset_prefix = if ignore_prefix { first_class_len } else { 0 }; | ||
| let offset_suffix = if ignore_suffix { last_class_len } else { 0 }; | ||
|
|
||
| let start = range.start() + TextSize::from(offset_prefix); | ||
| let end = range.end() - TextSize::from(offset_suffix); | ||
|
|
||
| if end < start { | ||
| return None; | ||
| } | ||
|
|
||
| Some(TextRange::new(start, end)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the guard comparison in both files
echo "=== JS Analyzer ==="
rg -n -C2 'if end' crates/biome_js_analyze/src/lint/nursery/use_sorted_classes/sort.rs
echo ""
echo "=== Tailwind Sorter ==="
rg -n -C2 'if end' crates/biome_tailwind_sort/src/sort.rsRepository: biomejs/biome
Length of output: 361
Align empty-range handling with the shared sorter.
The JS analyser permits zero-width ranges (end == start) to pass through, whilst the Tailwind sorter rejects them. Bring them into alignment to prevent fragile empty highlights.
- if end < start {
+ if end <= start {
return None;
}📝 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(crate) fn get_sort_class_name_range( | |
| class_name: &TokenText, | |
| range: &TextRange, | |
| template_ctx: &Option<TemplateLiteralSpaceContext>, | |
| ) -> Option<TextRange> { | |
| let sort_context = template_ctx.as_ref().map(|ctx| ctx.to_sort_context()); | |
| let mut class_iter = class_name.split_whitespace(); | |
| let first_class_len = class_iter.next().map_or(0, |s| s.len()) as u32; | |
| let last_class_len = class_iter.next_back().map_or(0, |s| s.len()) as u32; | |
| let (ignore_prefix, ignore_suffix) = sort_context | |
| .as_ref() | |
| .map_or((false, false), |ctx| (ctx.ignore_prefix, ctx.ignore_suffix)); | |
| let offset_prefix = if ignore_prefix { first_class_len } else { 0 }; | |
| let offset_suffix = if ignore_suffix { last_class_len } else { 0 }; | |
| let start = range.start() + TextSize::from(offset_prefix); | |
| let end = range.end() - TextSize::from(offset_suffix); | |
| if end < start { | |
| return None; | |
| } | |
| Some(TextRange::new(start, end)) | |
| } | |
| pub(crate) fn get_sort_class_name_range( | |
| class_name: &TokenText, | |
| range: &TextRange, | |
| template_ctx: &Option<TemplateLiteralSpaceContext>, | |
| ) -> Option<TextRange> { | |
| let sort_context = template_ctx.as_ref().map(|ctx| ctx.to_sort_context()); | |
| let mut class_iter = class_name.split_whitespace(); | |
| let first_class_len = class_iter.next().map_or(0, |s| s.len()) as u32; | |
| let last_class_len = class_iter.next_back().map_or(0, |s| s.len()) as u32; | |
| let (ignore_prefix, ignore_suffix) = sort_context | |
| .as_ref() | |
| .map_or((false, false), |ctx| (ctx.ignore_prefix, ctx.ignore_suffix)); | |
| let offset_prefix = if ignore_prefix { first_class_len } else { 0 }; | |
| let offset_suffix = if ignore_suffix { last_class_len } else { 0 }; | |
| let start = range.start() + TextSize::from(offset_prefix); | |
| let end = range.end() - TextSize::from(offset_suffix); | |
| if end <= start { | |
| return None; | |
| } | |
| Some(TextRange::new(start, end)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_js_analyze/src/lint/nursery/use_sorted_classes/sort.rs` around
lines 108 - 133, get_sort_class_name_range currently allows zero-width ranges
(end == start) to be returned; change the empty-range guard to reject
zero-length ranges by returning None when end <= start instead of only when end
< start so it matches the Tailwind sorter behavior; update the condition in
get_sort_class_name_range (the end/start check before Some(TextRange::new(start,
end))) to use <= and keep the rest of the logic (offset_prefix/offset_suffix and
TextRange::new) unchanged.
Merging this PR will not alter performance
Comparing Footnotes
|
|
I was planning on redoing that rule's logic so that we would have it in a |
|
My work doesn't void yours. The reason why I extracted this in a crate is because I plan to implement it in other languages (CSS, HTML), so a generic crate sounds a good solution |
|
Right, but the existing tailwind class sorting rule effectively uses it's own parser. I'm more concerned about my work voiding yours 😅 We would treat tailwind as an embedded language, and reimplement this rule in |
|
Ah, no, that was my plan all along. But since you're already on top of it, I leave that to you :) |
Summary
This PR moves the sorting algorithm we have in
use_sorted_classes.rsinto its own crate.I used an agent to do the move.
This is a refactor, so no changeset needed
Test Plan
Green CI
Docs