-
Notifications
You must be signed in to change notification settings - Fork 115
feat(iter): add iteri and iter2 #2012
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The iteri implementation mutates a captured variable which could be error-proneCategory pub fn Iter::iteri[T](self : Iter[T], offset~ : Int = 0) -> Iter2[Int, T] {
let enum_iter = self.enumerate()
enum_iter.map(fn((idx, val)) => (idx + offset, val))
}```
**Reasoning**
Using mutable state in iterators can be error-prone and harder to reason about. A more functional approach using map and enumerate would be clearer and potentially more efficient.
</details>
<details>
<summary> The iter2 function seems to be a special case of tuple destructuring that could be generalized </summary>
**Category**
Maintainability
**Code Snippet**
pub fn Iter::iter2[K, V](self : Iter[(K, V)]) -> Iter2[K, V] {
fn { yield_ => self.run(fn { (k, v) => yield_(k, v) }) }
}
**Recommendation**
Consider generalizing this into a tuple destructuring method or combining with other tuple operations
**Reasoning**
Having a specific method just for 2-tuples might lead to future requests for iter3, iter4 etc. A more general approach to tuple handling could be more maintainable.
</details>
<details>
<summary> Documentation could be more detailed about offset parameter behavior </summary>
**Category**
Maintainability
**Code Snippet**
/// Returns an iterator that iterates over the index and the element of the iterator.
**Recommendation**
Add more detailed documentation:
```moonbit
/// Returns an iterator that yields pairs of (index, element).
/// The index starts from the given offset (default 0).
///
/// Example:
/// for i, v in ["a", "b"].iter().iteri(offset=1) {
/// // yields (1,"a"), (2,"b")
/// } Reasoning |
Pull Request Test Coverage Report for Build 6460Details
💛 - Coveralls |
@@ -236,6 +236,8 @@ impl Iter { | |||
head[A](Self[A]) -> A? | |||
intersperse[A](Self[A], A) -> Self[A] | |||
iter[T](Self[T]) -> Self[T] | |||
iter2[K, V](Self[(K, V)]) -> Iter2[K, V] | |||
iteri[T](Self[T], offset~ : Int = ..) -> Iter2[Int, T] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iteri
looks reasonable.
What's the benefit of having iter2
, it doe not have perf benefit either?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have pattern matching for for .. in
, so one would have to write:
let map = [("a", "b"), ("c", "d")].iter()
for v in map {
let (k, v) = v
}
With iter2
, one can at least write:
for k, v in map.iter2() {
}
Closes #1319
The naming is chosen as such because:
mapi
eachi
where thei
signifies the fact that an index is added as parameterfor i, v in [].iter()
anyway, so there's no ambiguity.