Skip to content

feat(iter): add 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

Merged
merged 1 commit into from
May 30, 2025
Merged

feat(iter): add iter2 #2012

merged 1 commit into from
May 30, 2025

Conversation

peter-jerry-ye
Copy link
Collaborator

@peter-jerry-ye peter-jerry-ye commented Apr 27, 2025

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.

Update: we will use iter2 to return Iter2[Int, T] to align with existing behaviors. As of Iter[(K, V)], users can just use a pattern matching let (k, v) = elem.

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

peter-jerry-ye-code-review bot commented Apr 27, 2025

Initial index value could be confusing for empty iterators

Category
Correctness
Code Snippet
let mut index = -1
self.run(fn(i) {
index += 1
yield_(index, i)
})
Recommendation
Consider initializing index to 0 and incrementing after yield:

let mut index = 0
self.run(fn(i) {
  yield_(index, i)
  index += 1
})```
**Reasoning**
Starting with -1 and incrementing before use is less intuitive than starting with 0 and incrementing after use. Both approaches work but the recommended version is more straightforward to reason about, especially when debugging.

</details>
<details>

<summary> Test coverage could be more comprehensive </summary>

**Category**
Maintainability
**Code Snippet**
test "iter2" {
  let iter : Iter[Int] = [].iter()
  for _, _ in iter.iter2() {
    assert_true(false)
  }
  let iter = [0, 1, 2].iter()
  for i, x in iter.iter2() {
    assert_eq(i, x)
  }
}
**Recommendation**
Add tests for non-numeric types and check actual index values:
```moonbit
test "iter2_strings" {
  let iter = ["a", "b", "c"].iter()
  let mut expected_idx = 0
  for i, _ in iter.iter2() {
    assert_eq(i, expected_idx)
    expected_idx += 1
  }
}```
**Reasoning**
Current tests only cover numeric arrays where indices happen to match values. Testing with non-numeric types would ensure the indexing works correctly regardless of element type.

</details>
<details>

<summary> Documentation could be more detailed </summary>

**Category**
Maintainability
**Code Snippet**
///|
/// Returns an iterator that iterates over the index and the element of the iterator.
**Recommendation**
Add more detailed documentation with examples:
```moonbit
///|
/// Returns an iterator that yields pairs of indices and elements.
/// The indices start from 0 and increment by 1 for each element.
///
/// Example:
/// ```
/// let arr = ["a", "b", "c"].iter()
/// for i, v in arr.iter2() {
///   // i will be 0, 1, 2
///   // v will be "a", "b", "c"
/// }
/// ```
**Reasoning**
More detailed documentation with examples helps users understand the behavior and usage patterns more quickly, reducing potential misuse.

</details>

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

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 will use iter2 -> Iter2[Int, T]

@peter-jerry-ye
Copy link
Collaborator Author

@Guest0x0 we need to let for k, v in iter find iter2

@peter-jerry-ye peter-jerry-ye changed the title feat(iter): add iteri and iter2 feat(iter): add iter2 May 30, 2025
@peter-jerry-ye peter-jerry-ye enabled auto-merge (rebase) May 30, 2025 02:20
@peter-jerry-ye peter-jerry-ye merged commit e2e1e13 into main May 30, 2025
22 of 24 checks passed
@peter-jerry-ye peter-jerry-ye deleted the zihang/iter-2-iter2 branch May 30, 2025 03:49
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