Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Sep 25, 2025

This PR implements the missing Compare trait for @list.List, addressing the consistency issue where @immut/list.List had Compare support but @list.List did not.

Changes

  • Added Compare implementation: Implements lexicographic comparison for @list.List where elements are compared pairwise until a difference is found. When all shared elements are equal, the shorter list is considered less than the longer one.

  • Added comprehensive tests: Includes test cases covering lexicographic comparison, length-based comparison when prefixes are equal, equality comparison, and empty list edge cases.

  • Updated interface: The pkg.generated.mbti file now exposes impl[A : Compare] Compare for List[A] in the public API.

Implementation Details

The implementation follows lexicographic order semantics as clarified in the issue discussion:

let list1 = @list.of([1, 2, 3])
let list2 = @list.of([1, 2, 4])  
let list3 = @list.of([1, 2])

list1.compare(list2)  // -1 (lexicographic: 3 < 4)
list1.compare(list3)  // 1  (length: [1,2,3] > [1,2])
list3.compare(list1)  // -1 (length: [1,2] < [1,2,3])

This provides consistency with other data structures in the standard library and enables @list.List to be used in contexts requiring total ordering, such as sorted collections.

Fixes #[issue_number]

Original prompt

This section details on the original issue you should resolve

<issue_title>[Consistency Review] missing Compare impl for list.List</issue_title>
<issue_description>Current behaviors

There's no Compare implementation for @list.List. But there's Compare implementation for @immut/list.List

Expected behaviors

There's Compare implementation for @list.List

Justification
</issue_description>

<agent_instructions>Implement the Compare trait for the List under @list package. Follow the common implementations which is lexicographic order.</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@peter-jerry-ye No, it won't be shortlex.

With the Compare implementation, we only guarantee total order, so the Compare for the list will be lexicographic order instead of shortlex.</comment_new>
<comment_new>@peter-jerry-ye
Again, we don't have the consistency between data structures. The compare order is not guaranteed between Option[T] and Result[T, Unit].</comment_new>

---- **Additional instructions:**

Implement the Compare trait for the List under @list package. Follow the common implementations which is lexicographic order.

Fixes #2757


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Co-authored-by: peter-jerry-ye <16731997+peter-jerry-ye@users.noreply.github.com>
Copilot AI changed the title [WIP] [Consistency Review] missing Compare impl for list.List Implement Compare trait for @list.List with lexicographic order Sep 25, 2025
Copilot finished work on behalf of peter-jerry-ye September 25, 2025 06:46
@peter-jerry-ye peter-jerry-ye marked this pull request as ready for review September 25, 2025 06:48
@coveralls
Copy link
Collaborator

coveralls commented Sep 25, 2025

Pull Request Test Coverage Report for Build 1340

Details

  • 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.009%) to 89.13%

Totals Coverage Status
Change from base Build 1332: 0.009%
Covered Lines: 9290
Relevant Lines: 10423

💛 - Coveralls

(More(x, tail=xs), More(y, tail=ys)) => {
let cmp = x.compare(y)
if cmp != 0 {
break cmp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

break here is redundant.

Either

if cmp != 0 { break cmp }
continue (xs, ys)

or

if cmp != 0 { 
  cmp
} else {
  continue (xs, ys)
}

.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a lint rule for this. @myfreess

if x {
  return y // or other control flow transfer statement, e.g. break, raise, ...
} else {
  z
}

@peter-jerry-ye peter-jerry-ye merged commit ece9f4e into main Sep 25, 2025
28 checks passed
@peter-jerry-ye peter-jerry-ye deleted the copilot/fix-5988d796-b84e-48a1-8c4c-1f607c7c1f3e branch September 25, 2025 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Consistency Review] missing Compare impl for list.List

4 participants