Skip to content

Commit

Permalink
Fix DevinR528#72: --grouped handling when reording the first entry …
Browse files Browse the repository at this point in the history
…in a group.

DevinR528#72

Currently cargo-sort is broken with `--grouped` for situations where the first entry
in a group gets reordered within the group, such as the following example:

```toml
[package]
name = "foobar"
version = "0.1.0"
edition = "2021"

[dependencies]
a = { workspace = true }
b = { workspace = true }

d = { workspace = true }
c = { workspace = true }
```

Current behavior produces the following output:

```toml
[package]
name = "foobar"
version = "0.1.0"
edition = "2021"

[dependencies]
a = { workspace = true }
b = { workspace = true }
c = { workspace = true }

d = { workspace = true }
```

This is because the newline separating the groups is attached to the decor for `d`,
and it stays with `d` even as `c` and `d` change places.

This doesn't apply to the example above, but it's also possible for this behavior
to introduce non-idempotency since the new first item gets attached to the previous
group and it may get resorted within that group the next time cargo-sort is run.

I added a test for the correct behavior.
  • Loading branch information
Calsign committed Dec 20, 2024
1 parent f504796 commit 64fb9e2
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 2 deletions.
12 changes: 12 additions & 0 deletions examp/grouped_reorder.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
[package]
name = "foobar"
version = "0.1.0"
edition = "2021"

[dependencies]
a = { workspace = true }
c = { workspace = true }
b = { workspace = true }

e = { workspace = true }
d = { workspace = true }
12 changes: 12 additions & 0 deletions examp/grouped_reorder_output.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
[package]
name = "foobar"
version = "0.1.0"
edition = "2021"

[dependencies]
a = { workspace = true }
b = { workspace = true }
c = { workspace = true }

d = { workspace = true }
e = { workspace = true }
37 changes: 35 additions & 2 deletions src/sort.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ fn sort_by_group(table: &mut Table) {
table.clear();
let mut groups = BTreeMap::new();
let mut curr = 0;

for (idx, (k, v)) in table_clone.iter_mut().enumerate() {
let blank_lines = k
.leaf_decor()
Expand All @@ -175,18 +176,41 @@ fn sort_by_group(table: &mut Table) {
.filter(|l| !l.starts_with('#'))
.count();

let k = Key::new(&*k).with_leaf_decor(k.leaf_decor().clone());
let mut k = Key::new(&*k).with_leaf_decor(k.leaf_decor().clone());

if blank_lines > 0 {
// remove the newline from the beginning of the new group in case it gets moved to a
// different place in the group
let decor = k.leaf_decor_mut();
if let Some(prefix) = decor.prefix().and_then(RawString::as_str) {
if prefix.starts_with("\n") {
decor.set_prefix(RawString::from(&prefix[1..]));
} else if prefix.starts_with("\r\n") {
decor.set_prefix(RawString::from(&prefix[2..]));
}
}

groups.entry(idx).or_insert_with(|| vec![(k, v)]);
curr = idx;
} else {
groups.entry(curr).or_default().push((k, v));
}
}

for (_, mut group) in groups {
for (i, mut group) in groups.into_values().enumerate() {
group.sort_by(|a, b| a.0.cmp(&b.0));

// for sections after the first, attach a newline to the decor of the first element
if i != 0 {
if let Some((k, _)) = group.first_mut() {
let decor = k.leaf_decor_mut();
let prefix =
decor.prefix().and_then(RawString::as_str).unwrap_or_default();
// converting to clrf gets handled later if necessary
decor.set_prefix(RawString::from(format!("\n{prefix}")));
}
}

for (k, v) in group {
table.insert_formatted(&k, v.clone());
}
Expand Down Expand Up @@ -360,4 +384,13 @@ mod test {
);
assert_ne!(input, sorted.to_string());
}

#[test]
fn grouped_reorder() {
let input = fs::read_to_string("examp/grouped_reorder.toml").unwrap();
let expected_output =
fs::read_to_string("examp/grouped_reorder_output.toml").unwrap();
let sorted = super::sort_toml(&input, MATCHER, true, &[]);
assert_eq!(sorted.to_string(), expected_output);
}
}

0 comments on commit 64fb9e2

Please sign in to comment.