Skip to content

Commit

Permalink
Account for trailing comments in span handling
Browse files Browse the repository at this point in the history
Fixes #464

When using taplo rules to limit configuration to specific portions of a
document, trailing comments would break formatting.

`Node#text_ranges`, when considering tables, was returning ranges for
each interior element and the table header, which excluded trailing
comments. When the formatter walks the code and is trying to determine
which rules apply to a table, it checks whether the table is a subset of
the rule "span". When there is a trailing comment, this check fails, and
the rule will never apply.

Since the dom `Node` for tables contains only a reference to the table
header, we can get the text range of the entire table, including
trailing or interior comments, by traversing up to the parent and taking
its text range. If this is unavailable, the previous behavior is used.

Note that this modifies the behavior of `crate::dom::Node::text_ranges`,
which is part of the public API.

Signed-off-by: Clark Fischer <clark.fischer@gmail.com>
  • Loading branch information
clarkf committed Jan 3, 2024
1 parent 268c8b1 commit c3834c4
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 5 deletions.
52 changes: 47 additions & 5 deletions crates/taplo/src/dom/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,11 @@ impl Node {
Ok(all.into_iter())
}

/// Determine the portions of the source that correspond to this
/// `Node`.
///
/// Since a symbol may appear in multiple places, a single node
/// may correspond to multiple `TextRange`s.
pub fn text_ranges(&self) -> impl ExactSizeIterator<Item = TextRange> {
let mut ranges = Vec::with_capacity(1);

Expand All @@ -256,12 +261,22 @@ impl Node {
ranges.extend(entry.text_ranges());
}

if let Some(mut r) = v.syntax().map(|s| s.text_range()) {
for range in &ranges {
r = r.cover(*range);
}
// consider both v.syntax() and its parent node when
// broadening the range. v.syntax() is only the
// TableHeader, where its parent is the full Table
// syntax node.
let syntax_ranges = [
v.syntax().map(|s| s.text_range()),
v.syntax().and_then(|s| s.parent()).map(|n| n.text_range()),
];
for range in syntax_ranges {
if let Some(mut r) = range {
for range in &ranges {
r = r.cover(*range);
}

ranges.insert(0, r);
ranges.insert(0, r);
}
}
}
Node::Array(v) => {
Expand Down Expand Up @@ -659,3 +674,30 @@ impl From<Invalid> for Node {
Self::Invalid(v)
}
}

#[cfg(test)]
mod tests {
#[test]
fn test_dom_find_all_matches_includes_comments() {
let source = r#"
[table]
key = "value" # comment
"#;
let dom = crate::parser::parse(source).into_dom();
let matches = dom
.find_all_matches("table".parse().unwrap(), false)
.unwrap()
.collect::<Vec<_>>();
assert_eq!(1, matches.len());
let (_, node) = matches.first().unwrap();

// Ensure that at least one of the resultant spans includes
// the comment.
let spans = node.text_ranges().map(|r| &source[r]).collect::<Vec<_>>();
assert!(
spans.iter().any(|s| s.contains('#')),
"expected one of {:?} to contain a comment",
&spans
);
}
}
35 changes: 35 additions & 0 deletions crates/taplo/src/tests/formatter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1025,6 +1025,41 @@ foo = [
assert_format!(expected, &formatted);
}

/// See https://github.com/tamasfe/taplo/issues/464
#[test]
fn test_reorder_keys_trailing_comment() {
let src = r#"
[mytable]
a = "a"
c = "c"
b = "b" # ...
"#;

let expected = r#"
[mytable]
a = "a"
b = "b" # ...
c = "c"
"#;

let p = crate::parser::parse(src);
let formatted = crate::formatter::format_with_path_scopes(
p.into_dom(),
Default::default(),
&[],
vec![(
"mytable",
formatter::OptionsIncomplete {
reorder_keys: Some(true),
..Default::default()
},
)],
)
.unwrap();

assert_format!(expected, &formatted);
}

#[test]
fn test_single_comment_no_alignment() {
let src = r#"
Expand Down

0 comments on commit c3834c4

Please sign in to comment.