keep-sorted icon indicating copy to clipboard operation
keep-sorted copied to clipboard

keep-sorted could handle trailing commas better

Open JeffFaer opened this issue 1 year ago • 1 comments

There's some logic in keep-sorted to try and gracefully handle lists that don't have a final trailing comma like

ImmutableList.of(
  // keep-sorted start
  1,
  3,
  2
  // keep-sorted end
);

would become the following (commas added and removed as necessary)

ImmutableList.of(
  // keep-sorted start
  1,
  2,
  3
  // keep-sorted end
);

https://github.com/google/keep-sorted/blob/62dd1440b1c8456b372bff09161ea9d847247671/keepsorted/block.go#L338-L340

There's a couple edge cases here that aren't handled very well right now

  1. Trailing comments after a comma

    This prevents the special case from being triggered

    ImmutableList.of(
      // keep-sorted start
      1,
      3, // three
      2
      // keep-sorted end
    );
    

    would become

    ImmutableList.of(
      // keep-sorted start
      1,
      2
      3, // three
      // keep-sorted end
    );
    
  2. Trailing comment after the last line

    The logic still triggers, but we add a comma to the comment

    ImmutableList.of(
      // keep-sorted start
      1,
      3, 
      2 // two
      // keep-sorted end
    );
    

    would become

    ImmutableList.of(
      // keep-sorted start
      1,
      2 // two,
      3
      // keep-sorted end
    );
    

JeffFaer avatar Apr 03 '24 19:04 JeffFaer

Here's my take on both handling this and generalizing it to avoid hard-coding one-off logic concepts: https://github.com/AlexanderGH/presubmitchecks/pull/7

It's not an exact 1:1 because it doesn't support stuff like this: https://github.com/google/keep-sorted/blob/8d641518728d604fd965ca442704d0356912e76b/goldens/prefixes.in#L16 (it would maintain the missing comma in the same position) but if i wanted to, i could do a pre-sort by prefix order, but that seems like too much of a special-case and trying to again put language knowledge into the system.

tl;dr: accept a regexp with two subgroups. one subgroup is sticky to the sortable group, and the other is sticky to the position (anything outside the subgroups is dropped). In this example the comma would be sticky to the group position and the comment sticky to the sortable group.

AlexanderGH avatar Jan 26 '25 01:01 AlexanderGH