fix: Don't swallow trailing whitespace of merged nodes #59

wetneb · 2024-11-26T17:41:40+01:00

wetneb commented

2024-11-26 17:41:40 +01:00

Fixes #58.

This is something I also encountered when working on Kotlin support (#56), but since the problem is also present in TOML I think it's worth backporting.
Opening the PR with the failing test case first to demonstrate the failure.

Cc @funkeleinhorn @ugur-a

Fixes #58. This is something I also encountered when working on Kotlin support (#56), but since the problem is also present in TOML I think it's worth backporting. Opening the PR with the failing test case first to demonstrate the failure. Cc @funkeleinhorn @ugur-a

wetneb added 1 commit

2024-11-26 17:41:40 +01:00

Add failing example for issue #58

/ rustfmt (pull_request) Successful in 21s

Details

/ test (pull_request) Failing after 38s

Details

962fbad067

wetneb added 1 commit

2024-11-26 17:51:58 +01:00

Fix #58 by adding trailing whitespace handling

/ rustfmt (pull_request) Successful in 22s

Details

/ test (pull_request) Successful in 36s

Details

c28438235a

wetneb reviewed

2024-11-26 17:58:40 +01:00

src/merged_tree.rs

					
				@ -513,0 +532,4 @@

				            }

				            (_, Some(node), _) | (_, _, Some(node)) | (Some(node), _, _) => {

				                node.trailing_whitespace()

				            }

wetneb commented

2024-11-26 17:58:40 +01:00

This is a bit of a complicated logic: the three versions of a given node (base, left, right) can have different trailing whitespaces, and this heuristic attempts to pick the most sensible one:

if the node is present in all three revisions, we try to handle pure reformattings: if base and left are equal, then go for right, if base and right are equal go for left, and in all other cases prefer left (arbitrarily)
otherwise, prefer left, right and base in that order.

Happy to make adjustments if you can think of a better idea…

This is a bit of a complicated logic: the three versions of a given node (base, left, right) can have different trailing whitespaces, and this heuristic attempts to pick the most sensible one: * if the node is present in all three revisions, we try to handle pure reformattings: if base and left are equal, then go for right, if base and right are equal go for left, and in all other cases prefer left (arbitrarily) * otherwise, prefer left, right and base in that order. Happy to make adjustments if you can think of a better idea…

ugur-a commented

2024-11-29 10:31:49 +01:00

This looks kind of similar to

                 let (preceding_whitespace, indentation_shift) = match whitespaces {
                     [Some(whitespace_left), Some(whitespace_right), Some(whitespace_base)] => {
                         if whitespace_base == whitespace_left {
                             whitespace_right
                         } else {
                             whitespace_left
                         }
                     }
                     [Some(w), _, _] | [_, Some(w), _] | [_, _, Some(w)] => w,
                     _ => representatives
                         .iter()
                         .find_map(|repr| {
                             let indentation_shift =
                                 repr.node.indentation_shift().unwrap_or("").to_owned();
                             let ancestor_newlines =
                                 format!("\n{}", repr.node.ancestor_indentation().unwrap_or(""));
                             let new_newlines = format!("\n{indentation}");
                             if let Some(preceding_whitespace) = repr.node.preceding_whitespace() {
                                 let new_whitespace =
                                     preceding_whitespace.replace(&ancestor_newlines, &new_newlines);
                                 Some((new_whitespace, indentation_shift))
                             } else {
                                 None
                             }
                         })
                         .unwrap_or_default(),
                 };

now that #62 is merged, so I'd say the logic is actually okay and also consistent. So LGTM!

This looks kind of similar to https://codeberg.org/mergiraf/mergiraf/src/commit/10361193c1b817c53006248d2a8ba5e2af08f6d6/src/merged_tree.rs#L420-L446 now that https://codeberg.org/mergiraf/mergiraf/pulls/62 is merged, so I'd say the logic is actually okay and also consistent. So LGTM!

wetneb commented

2024-11-29 13:12:38 +01:00

Yes, I also noticed that your new version makes it easier to specify the priority of revisions if necessary, which can be useful.