Skip to content

Conversation

Takashiidobe
Copy link
Contributor

Examples for minmax_by and minmax_by_key.

Copy link

codecov bot commented Oct 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.67%. Comparing base (6814180) to head (7a22d97).
⚠️ Report is 167 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1064      +/-   ##
==========================================
- Coverage   94.38%   93.67%   -0.72%     
==========================================
  Files          48       50       +2     
  Lines        6665     6327     -338     
==========================================
- Hits         6291     5927     -364     
- Misses        374      400      +26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@phimuemue phimuemue left a comment

Choose a reason for hiding this comment

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

Thanks for spotting the missing examples, but could you please replace them by something that uses (1, 'a'), (1, 'b'), (1, 'c'), (0, 'a'), (0, 'b'), (0, 'c'), and replace abs_key=... by |(number, character)| number (or equivalent). Then, the "for minimum, first minimal element wins and for maximum, last maximal element wins" is clearer. (Or, remove the 0 so that -1 or 1 is contained in the resulting MinMax, whichever is clearer to you.)

…ke it clearer which minimum elements and maximum elements are chosen
@Takashiidobe
Copy link
Contributor Author

Takashiidobe commented Oct 9, 2025

I removed the absolute value comparator and changed the items to (i32, char) so it was clearer which elements are chosen by minmax. I also changed the minmax example cause it's not clear which 1 wins in the last example of a slice of 1s.

@Takashiidobe Takashiidobe requested a review from phimuemue October 9, 2025 17:37
Comment on lines 4074 to 4084
/// let a: [i32; 0] = [];
/// let a: [(i32, char); 0] = [];
/// assert_eq!(a.iter().minmax(), NoElements);
///
/// let a = [1];
/// assert_eq!(a.iter().minmax(), OneElement(&1));
/// let a = [(1, 'a')];
/// assert_eq!(a.iter().minmax(), OneElement(&(1, 'a')));
///
/// let a = [1, 2, 3, 4, 5];
/// assert_eq!(a.iter().minmax(), MinMax(&1, &5));
/// let a = [(0, 'a'), (1, 'b')];
/// assert_eq!(a.iter().minmax(), MinMax(&(0, 'a'), &(1, 'b')));
///
/// let a = [1, 1, 1, 1];
/// assert_eq!(a.iter().minmax(), MinMax(&1, &1));
/// let a = [(1, 'a'), (1, 'b'), (1, 'c')];
/// assert_eq!(a.iter().minmax(), MinMax(&(1, 'a'), &(1, 'c')));
Copy link
Member

Choose a reason for hiding this comment

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

Please leave them simple numbers, and add one representative example with tuples as an extra case. (Tuple comparisons are harder to understand, so we should keep the simple cases.)

src/lib.rs Outdated
/// use itertools::Itertools;
/// use itertools::MinMaxResult::{MinMax, NoElements, OneElement};
///
/// let abs_cmp = |x: &&(i32, char), y: &&(i32, char)| x.0.cmp(&y.0);
Copy link
Member

Choose a reason for hiding this comment

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

abs_cmp does not seem a reasonable name for the closure.

Copy link
Member

@phimuemue phimuemue left a comment

Choose a reason for hiding this comment

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

Thanks for the follow-up, but I still have some nits - sorry for that, but if we already touch it, I don't want to stop half-way.

@Takashiidobe
Copy link
Contributor Author

Fixed the closure name and restored the old example for minmax.

@Takashiidobe Takashiidobe requested a review from phimuemue October 9, 2025 18:18
Copy link
Member

@phimuemue phimuemue left a comment

Choose a reason for hiding this comment

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

Thank you!

@phimuemue phimuemue added this pull request to the merge queue Oct 9, 2025
Merged via the queue into rust-itertools:master with commit 6bd5053 Oct 9, 2025
12 of 14 checks passed
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.

2 participants