Add array_combinations_with_replacement
#1033
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1033 +/- ##
==========================================
- Coverage 94.38% 94.24% -0.14%
==========================================
Files 48 50 +2
Lines 6665 6226 -439
==========================================
- Hits 6291 5868 -423
+ Misses 374 358 -16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/combinations_with_replacement.rs
Outdated
|
|
||
| fn next(&mut self) -> Option<Self::Item> { | ||
| if self.first { | ||
| // In empty edge cases, stop iterating immediately | ||
| if !(self.indices.is_empty() || self.pool.get_next()) { | ||
| if !(core::borrow::Borrow::<[usize]>::borrow(&self.indices).is_empty() |
There was a problem hiding this comment.
Can't we write self.indices.borrow()?
There was a problem hiding this comment.
Weird, that work, when I originally tried doing that I got an error, but now it works.
| @@ -24,6 +24,10 @@ where | |||
|
|
|||
| /// Iterator for `Box<[I]>` valued combinations_with_replacement returned by [`.combinations_with_replacement()`](crate::Itertools::combinations_with_replacement) | |||
| pub type CombinationsWithReplacement<I> = CombinationsWithReplacementGeneric<I, Box<[usize]>>; | |||
| /// Iterator for const generic combinations_with_replacement returned by [`.array_combinations_with_replacement()`](crate::Itertools::array_combinations_with_replacement) | |||
There was a problem hiding this comment.
Non-markdown expert here: Is the (crate::Itertools::array_combinations_with_replacement) required?
There was a problem hiding this comment.
I don't know I just copied and modified the doc comment from two lines above.
The whole combinations_with_replacement file seems to feature gated under Edit: |
|
I added test for |
(and its dependencies)
There was a problem hiding this comment.
Thank you. Could you fix so that CI runs through?
Can you limit the cfgs to where they are necessary please?
And by the way: Thanks for splitting up the PR into nicely diffable chunks, and for just amending instead of force-pushing and overwriting old commits.
| @@ -52,7 +52,16 @@ pub trait PoolIndex<T>: BorrowMut<[usize]> { | |||
| self.borrow().len() | |||
| } | |||
| } | |||
| impl<T> PoolIndex<T> for Box<[usize]> { | |||
| type Item = Vec<T>; | |||
There was a problem hiding this comment.
We can think about returning Box<[T]> later, but since we'd probably construct it via a Vec, Vec is fine.
There was a problem hiding this comment.
I just what did not change that, as that was the way it was before.
src/combinations_with_replacement.rs
Outdated
| @@ -1,3 +1,4 @@ | |||
| #[cfg(feature = "use_alloc")] | |||
There was a problem hiding this comment.
I think cfg(feature="use_alloc") in combinations_with_replacement.rs is redundant because the module itself is only conditionally (see https://github.com/rust-itertools/itertools/blob/master/src/lib.rs#L101-L102). Please double-check and remove them if that's the case.
There was a problem hiding this comment.
Yeah I see, I was getting confused because of a missing Box import, when writing it.
|
Maybe we should remove the |
`combinations_with_replacements` as they are done at a module level right now.
This PR adds a new method for iterators called
array_combinations_with_replacementwhich is similar tocombinations_with_replacement, except for that it uses const generics and arrays.This is similar to
array_combinationsandcombinations.I also generalized the underlying
CombinationsWithReplacementtype similar to how it is done withCombinations.This closes #1025.