8000 change return type of @sorted_map.keys/values to Iter by FlyCloudC · Pull Request #2123 · moonbitlang/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

change return type of @sorted_map.keys/values to Iter #2123

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

change return type of @sorted_map.keys/values to Iter #2123

wants to merge 3 commits into from

Conversation

FlyCloudC
Copy link
Contributor

see #2108

@peter-jerry-ye-code-review peter-jerry-ye code review
Copy link
peter-jerry-ye-code-review bot commented May 18, 2025 •
edited
Loading
Change keys(), values(), and elems() to return iterators instead of arrays for better memory efficiency

Category
Performance
Code Snippet
pub fn keys[K, V](self : T[K, V]) -> Iter[K]
pub fn values[K, V](self : T[K, V]) -> Iter[V]
Recommendation
The change from Array to Iter is good. Consider adding a note in documentation about the change in behavior, as some callers might expect to chain array operations.
Reasoning
Returning iterators instead of eagerly constructing arrays improves memory efficiency when only iteration is needed. It also allows for short-circuiting operations in chains. This is a common optimization pattern in modern collections APIs.

Documentation of test case name doesn't match functionality

Category
Maintainability
Code Snippet
test "to_array" {
let m = @sorted_map.of([(1, "one"), (2, "two"), (3, "three")])
assert_eq!(m.iter().collect(), [(1, "one"), (2, "two"), (3, "three")])
}
Recommendation
Change test name from "to_array" to "iter" to match the functionality being tested
Reasoning
The test name should reflect what is actually being tested. This test is verifying the iter() functionality rather than to_array(), making the current name misleading for future maintainers.

All test assertions updated to call collect() on iterators

Category
Correctness
Code Snippet
assert_eq!(map.elems(), [1, 2, 3]) -> assert_eq!(map.elems().collect(), [1, 2, 3])
Recommendation
The changes are correct. Consider adding test cases that use the iterator without collecting to verify lazy evaluation works correctly.
Reasoning
While the test updates are necessary due to the API change, additional tests for the lazy behavior would help ensure the performance benefits are actually realized.

@coveralls
Copy link
Collaborator
coveralls commented May 18, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 6855

Details

  • 16 of 20 (80.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.04%) to 91.191%
Changes Missing Coverage Covered Lines Changed/Added Lines %
sorted_map/map.mbt 14 18 77.78%
Totals Coverage Status
Change from base Build 6851: -0.04%
Covered Lines: 6346
Relevant Lines: 6959
💛 - Coveralls

@FlyCloudC FlyCloudC force-pushed the xxmap-keys-values branch from cdbb06e to 5c174ea Compare May 18, 2025 14:40
@FlyCloudC FlyCloudC marked this pull request as ready for review May 18, 2025 15:03
@FlyCloudC FlyCloudC mentioned this pull request May 18, 2025
Open
@peter-jerry-ye
Copy link
Collaborator

Same as #2124, this introduces breaking changes. It would be better to have some migration process for users.

@FlyCloudC
Copy link
Contributor Author

Thanks for the feedback. I’d like to ensure we handle the migration correctly. Since this is a return type change (not just a rename), could you clarify the preferred way to deprecate the old behavior while maintaining backward compatibility?

I noticed PR #1861 made a similar change directly—should we follow that precedent, or is there a different pattern you’d recommend for this case?

@FlyCloudC FlyCloudC closed this May 19, 2025
@FlyCloudC FlyCloudC force-pushed the xxmap-keys-values branch from 5c174ea to c2f9399 Compare May 19, 2025 09:46
@FlyCloudC FlyCloudC reopened this May 19, 2025
@FlyCloudC FlyCloudC force-pushed the xxmap-keys-values branch from b3df260 to 389ff77 Compare May 19, 2025 15:28
@peter-jerry-ye
Copy link
Collaborator

Yeah, I admit that #1861 did it abruptly. If possible, I would like you to define a new method, probably something like keys_as_iter, and add deprecation to the keys and values to let people know about that this is actually happening.

@FlyCloudC FlyCloudC marked this pull request as draft May 20, 2025 12:44
@FlyCloudC
Copy link
Contributor Author

Waiting for #2124 to be merged first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@FlyCloudC @coveralls @peter-jerry-ye @myfreess
0