Skip to content

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Apr 19, 2025

  • Added a CHANGELOG.md entry

Fixes #1620 by:

  • Renaming IndexedRandom::choose_multiple*sample*
  • Renaming IteratorRandom::choose_multiple*sample*
  • Adding IndexedRandom::choose_iter, choose_weighted_iter

I think I'm happy to go with this. @vks, @benjamin-lieser review please.

@dhardy dhardy force-pushed the push-kvlpklxwrypt branch from e67fa45 to 534b292 Compare April 19, 2025 12:14
@dhardy
Copy link
Member Author

dhardy commented Apr 19, 2025

Note: I don't think we should add choose_mut_iter. Even if we wanted to, we couldn't do so using the Iterator trait; we'd need a streaming/borrowing iterator trait. The choose_iter fn here returns the same lifetime (that of the iterator) for each next sample.

I briefly considered deprecating all the choose_mut fns but a GitHub search shows some uses.

@dhardy dhardy requested review from vks and benjamin-lieser April 25, 2025 07:51
@dhardy dhardy added the B-API Breakage: API label Jun 1, 2025
dhardy added a commit that referenced this pull request Aug 12, 2025
@dhardy dhardy mentioned this pull request Aug 12, 2025
1 task
@dhardy
Copy link
Member Author

dhardy commented Sep 4, 2025

Rebased. Ready for merge (hopefully).

@dhardy dhardy merged commit 34b2e45 into master Sep 4, 2025
16 checks passed
@dhardy dhardy deleted the push-kvlpklxwrypt branch September 4, 2025 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-API Breakage: API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

choose_multiple should return Result or Option
2 participants