Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

peter-jerry-ye
Copy link
Collaborator

Closes #1319

The naming is chosen as such because:

  • we have mapi eachi where the i signifies the fact that an index is added as parameter
  • we can't write for i, v in [].iter() anyway, so there's no ambiguity.

@peter-jerry-ye peter-jerry-ye requested a review from bobzhang April 27, 2025 06:33
Copy link

The iteri implementation mutates a captured variable which could be error-prone

Category
Correctness
Code Snippet
pub fn Iter::iteri[T](self : Iter[T], offset~ : Int = 0) -> Iter2[Int, T] {
fn {
yield_ => {
let mut value = offset - 1
self.run(fn(i) {
value += 1
yield_(value, i)
})
}
}
}
Recommendation
Consider using a more functional approach with map:

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
The current documentation doesn't explain the offset parameter or provide examples, which could lead to confusion about its behavior.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 6460

Details

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

Totals Coverage Status
Change from base Build 6455: 0.02%
Covered Lines: 6124
Relevant Lines: 6607

💛 - 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]
Copy link
Contributor

@bobzhang bobzhang Apr 28, 2025

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?

Copy link
Collaborator Author

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() {
}

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.

[Feature Request] impl Iter2 for Iter[(A, B)]
3 participants