feat: add @cmp.(max|min)imum_by_key

[rami3l]

Mirrors Rust's max_by_key and min_by_key functions.

[peter-jerry-ye-code-review]

‼️ This code review is generated by a bot. Please verify the content before trusting it.

⚠️ [Inconsistent handling of equality in `minimum_by_key` and `maximum_by_key`]

• Category: Correctness
• Code Snippet:

// maximum_by_key
if f(x) > f(y) {
  x
} else {
  y
}

// minimum_by_key
if f(x) > f(y) {
  y
} else {
  x
}

• Recommendation: Ensure both functions handle equality explicitly and consistently. For minimum_by_key, it should return x when f(x) <= f(y) to match maximum_by_key logic.
• Reasoning: The documentation for minimum_by_key states it should return the first argument when equal, but the implementation returns x when f(x) <= f(y). This might be counterintuitive and inconsistent with maximum_by_key behavior.

💡 [Consider adding edge case tests]

• Category: Maintainability
• Code Snippet:

test "minimum_by_key" {
  assert_eq!(@cmp.minimum_by_key(1, -2, @int.abs), 1)
  assert_eq!(@cmp.minimum_by_key(-2, 1, @int.abs), 1)
  assert_eq!(@cmp.minimum_by_key(-2, 2, @int.abs), -2)
}

• Recommendation: Add tests for edge cases where inputs are equal (f(x) == f(y)) and for different data types to ensure the functions work as expected in all scenarios.
• Reasoning: While the current tests cover the basic functionality, adding tests for edge cases would make the test suite more robust and help catch potential issues in the future.

💡 [Consider adding inline documentation for generics]

• Category: Style Guide Compliance
• Code Snippet:

pub fn maximum_by_key[T, K : Compare](x : T, y : T, f : (T) -> K) -> T {

• Recommendation: Add documentation for the generic types T and K to clarify their roles and constraints.
• Reasoning: While the function documentation describes the overall behavior, being explicit about the generics would make the code more self-documenting and easier to understand for new developers.

[coveralls]

Pull Request Test Coverage Report for Build 5865

Details

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

---

Totals
Change from base Build 5854:	0.008%
Covered Lines:	5594
Relevant Lines:	6051

---

💛 - Coveralls

[bobzhang]

Do we need these tiny functions? I think they are not particularly useful

[rami3l]

Do we need these tiny functions? I think they are not particularly useful

I think it makes some business logic more readable, like the example below:

Before

let span l0 l1 =
  let first_byte, first_line =
    if l0.first_byte < l1.first_byte
    then l0.first_byte, l0.first_line
    else l1.first_byte, l1.first_line
  in
  let last_byte, last_line, file =
    if l0.last_byte < l1.last_byte
    then l1.last_byte, l1.last_line, l1.file
    else l0.last_byte, l0.last_line, l0.file
  in
  v ~file ~first_byte ~first_line ~last_byte ~last_line

After

pub fn TextLoc::span(self : TextLoc, other : TextLoc) -> TextLoc {
  let { first_byte, first_line, .. } = minimum_by_key(other, self, fn(it) { it.first_byte })
  let { file, last_byte, last_line, .. } = maximum_by_key(other, self, fn(it) { it.last_byte })
  { file, first_byte, last_byte, first_line, last_line }
}

[rami3l]

Python does the same thing with the key parameter in max(arg1, arg2, *args, key=None).

Similarly, in the OCaml ecosystem, a slightly more heavyweight solution is available via Comparator.

[bobzhang]

@rami3l I think it is okay to have this util, but it does not belong to math package, can you create a cmp package like rust or Fun module like OCaml?

[rami3l]

@rami3l I think it is okay to have this util, but it does not belong to math package, can you create a cmp package like rust or Fun module like OCaml?

@bobzhang Thanks for the feedback! In that case, however, should @math.(max|min) be moved elsewhere as well?

PS: I believe the Fun module in OCaml is for general operators on lambdas... cmp looks good to me.

[rami3l]

@bobzhang Moved to @cmp as previously discussed.