feat: add format range functionality for partial code formatting#48
feat: add format range functionality for partial code formatting#48magic-akari wants to merge 1 commit into
Conversation
Implement the ability to format specific ranges of CSS code instead of entire files, enabling editor integrations where users can select and format only portions of code.
There was a problem hiding this comment.
Pull request overview
This PR implements range formatting functionality for Malva, enabling users to format specific portions of CSS/SCSS/Sass/Less code rather than entire files. This is particularly valuable for editor integrations where users select and format only a portion of their code.
Key changes:
- Implements
format_rangefunction that intelligently expands selections to complete syntactic units (statements or declarations) - Calculates and preserves base indentation levels for formatted ranges
- Adds comprehensive test suite with snapshot testing across multiple CSS dialects
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| malva/src/range.rs | Core implementation of range formatting logic with node detection and indent calculation |
| malva/src/lib.rs | Exports public API for format_range function and FormatRangeResult type |
| malva/src/error.rs | Adds RangeOutOfBounds error variant for range validation |
| malva/tests/fmt_range.rs | Comprehensive test suite including unit tests and snapshot tests |
| malva/tests/fmt_range/**/*.{css,scss,less} | Test fixtures for range formatting |
| malva/tests/fmt_range/**/*.range | Range specification files for test cases |
| malva/tests/fmt_range/**/*.snap | Expected output snapshots for test cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Wrap with base indent | ||
| let doc = doc.nest(base_indent); | ||
|
|
||
| // Print => doc |
There was a problem hiding this comment.
The comment "Print => doc" appears to be backwards or confusing. It should likely say "Print doc" or "Print the doc" since this is converting the doc to a string, not converting print to doc.
| // Print => doc | |
| // Print doc |
|
|
||
| let output = format_range(&input, start..end, syntax, &options).unwrap(); | ||
| let output = { | ||
| let mut result = input.clone(); |
There was a problem hiding this comment.
The variable input is a string slice, so calling .clone() on line 119 is unnecessary and potentially misleading. Since input is an &str, you should use .to_string() or just construct the result directly without the intermediate clone.
| let mut result = input.clone(); | |
| let mut result = input.to_string(); |
| fn find_range_node<'a, 's>( | ||
| stylesheet: &'a Stylesheet<'s>, | ||
| range: &Range<usize>, | ||
| _source: &str, |
There was a problem hiding this comment.
The parameter _source is prefixed with an underscore indicating it's unused, but it's included in the function signature. If this parameter is genuinely not needed, it should be removed to avoid confusion. If it's kept for future use or consistency, a comment explaining why would be helpful.
| _source: &str, | |
| _source: &str, // kept for API compatibility / potential future use; intentionally unused |
|
|
||
| } | ||
| @function grid-width($n) { | ||
| @return $very-very-very-very-very-very-vey-long-var * $very-very-very-very-very-very-vey-long-var + ($very-very-very-very-very-very-vey-long-var - 1) * $very-very-very-very-very-very-vey-long-var; |
There was a problem hiding this comment.
The word "vey" appears to be a typo and should be "very". This typo is repeated multiple times in this line.
|
|
||
| } | ||
| @function grid-width($n) { | ||
| @return $very-very-very-very-very-very-vey-long-var * $very-very-very-very-very-very-vey-long-var + ($very-very-very-very-very-very-vey-long-var - 1) * $very-very-very-very-very-very-vey-long-var; |
There was a problem hiding this comment.
The word "vey" appears to be a typo and should be "very". This typo is repeated multiple times in this line.
|
|
||
| /// Represents a node or a list of sibling nodes that should be formatted together. | ||
| enum RangeNode<'a, 's> { | ||
| /// No formatable node found in the range. |
There was a problem hiding this comment.
The word "formatable" should be spelled "formattable" with double 't'.
| /// No formatable node found in the range. | |
| /// No formattable node found in the range. |
| /// - A single node that completely contains the range, or | ||
| /// - Multiple sibling nodes that together contain the range | ||
| /// | ||
| /// For CSS, the minimum formatable unit is a "line-level" node: |
There was a problem hiding this comment.
The word "formatable" should be spelled "formattable" with double 't'.
| /// For CSS, the minimum formatable unit is a "line-level" node: | |
| /// For CSS, the minimum formattable unit is a "line-level" node: |
There was a problem hiding this comment.
Declaration is also one kind of statement, so it can be merged in RangeNode.
Actually statements can be nested in any level depth, but it seems this PR only processes two level.
To be honest, due to the design of Raffia, range formatting is hard to to be implemented, and I don't consider this feature until it's highly requested. I don't think it's a rare use case but also I don't think developers can't live without this.
| options: &FormatOptions, | ||
| ) -> Result<FormatRangeResult, Error> { | ||
| // 1. Validate range | ||
| if range.start > source.len() || range.end > source.len() { |
There was a problem hiding this comment.
| if range.start > source.len() || range.end > source.len() { | |
| if !range.contains(source.len()) { |
| /// The specified range is outside of the source file bounds. | ||
| RangeOutOfBounds { | ||
| range: std::ops::Range<usize>, | ||
| source_len: usize, |
There was a problem hiding this comment.
| source_len: usize, | |
| total: usize, |
| } | ||
|
|
||
| impl RangeNode<'_, '_> { | ||
| /// Get the span of this range node. |
There was a problem hiding this comment.
| /// Get the span of this range node. |
| let statements = &stylesheet.statements; | ||
|
|
||
| // Check if the range covers the entire file | ||
| if range.start == 0 && range.end >= stylesheet.span().end { |
There was a problem hiding this comment.
| if range.start == 0 && range.end >= stylesheet.span().end { | |
| if range.start == 0 && range.end >= stylesheet.span.end { |
| // Check if the range covers the entire file | ||
| if range.start == 0 && range.end >= stylesheet.span().end { | ||
| if let Some(first) = statements.first() { | ||
| return (RangeNode::MultipleStatements(statements.iter().collect()), first.span().clone()); |
There was a problem hiding this comment.
| return (RangeNode::MultipleStatements(statements.iter().collect()), first.span().clone()); | |
| return (RangeNode::MultipleStatements(statements.clone()), first.span().clone()); |
| let (range_node, ref_span) = find_range_node(&stylesheet, &range, source); | ||
|
|
||
| // 4. Calculate base indentation from reference span | ||
| let base_indent = calculate_base_indent(ref_span, source, options); |
There was a problem hiding this comment.
We don't need to do such complicated calculation. A simple solution: pick span of first statement, then get its column by using line_bounds.get_line_col. The column value will be the base indent.
| let mut docs = Vec::with_capacity(stmts.len() * 2); | ||
| for (i, stmt) in stmts.iter().enumerate() { | ||
| if i > 0 { | ||
| docs.push(Doc::hard_line()); |
There was a problem hiding this comment.
What about comments and consecutive line breaks?
I see your point. When implementing this, I looked at how ruff and biome handle it. The main challenge, I believe, is tracking positions in the output, largely due to the design of tiny_pretty. Since CSS nesting levels are usually shallow, I tried implementing it by leveraging the text positions of input nodes along with some assumptions. I'd like to hear your thoughts on this: would you prefer seeing this feature integrated into the lower-level infrastructure, or would you rather wait for more user demand before investing more effort into it? If you'd rather not pursue this further at the moment, I'm happy to put the PR on hold. |
This shouldn't be related to tiny_pretty. It should be Raffia, the parser.
I prefer waiting. |
Implement the ability to format specific ranges of CSS code instead of entire files, enabling editor integrations where users can select and format only portions of code.