Skip to content

Conversation

Asterless
Copy link
Contributor

@Asterless Asterless commented Jun 6, 2025

Summary

This PR supplements the higher-order functions that were omitted in #2211

Details

Affected Functions

  • Array

    • makei
  • FixedArray

    • each
    • makei
    • rev_each
  • View

    • map
    • mapi

These updates ensure a more robust and consistent approach to error handling across the array-related modules.

Copy link

peter-jerry-ye-code-review bot commented Jun 6, 2025

Incorrect early return type in Array::makei implementation

Category
Correctness
Code Snippet
pub fn[T] Array::makei(length : Int, value : (Int) -> T?Error) -> Array[T]?Error {
if length <= 0 {
[]
} else {
Recommendation
Change early return to be explicit about error handling:

if length <= 0 {
  return ok([])
} else {

Reasoning
Since the function signature promises Array[T]?Error return type, returning just [] without wrapping in ok() is incorrect and inconsistent with error handling pattern. This could cause type checking issues.

Missing error propagation in View::map loop implementation

Category
Maintainability
Code Snippet
pub fn[T, U] View::map(self : View[T], f : (T) -> U?Error) -> Array[U]?Error {
if self.length() == 0 {
return []
}
Recommendation
Add proper error handling in the implementation:

if self.length() == 0 {
  return ok([])
}

Reasoning
The function now accepts callbacks that can fail with errors, but the implementation doesn't show how these errors are propagated. Need to ensure errors from the callback are properly handled and propagated.

Inconsistent documentation examples with new error handling

Category
Maintainability
Code Snippet
/// /// [1, 2, 3, 4, 5].each(fn(x){ arr.push(x) }) /// assert_eq(arr, [1, 2, 3, 4, 5]) ///
Recommendation
Update documentation examples to reflect error handling:

/// ```
/// try {
///   [1, 2, 3, 4, 5].each(fn(x){ arr.push(x) })
///   assert_eq(arr, [1, 2, 3, 4, 5])
/// }
/// ```
**Reasoning**
The documentation examples don't reflect the new error handling capabilities of these functions. Examples should demonstrate proper error handling to guide developers in correct usage.

</details>

@coveralls
Copy link
Collaborator

coveralls commented Jun 6, 2025

Pull Request Test Coverage Report for Build 7193

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.105%

Totals Coverage Status
Change from base Build 7188: 0.0%
Covered Lines: 8574
Relevant Lines: 9209

💛 - Coveralls

@hackwaly hackwaly requested a review from peter-jerry-ye June 9, 2025 07:11
@peter-jerry-ye peter-jerry-ye requested review from Yu-zh and removed request for peter-jerry-ye June 10, 2025 02:11
@peter-jerry-ye peter-jerry-ye force-pushed the Refactor/@Array-ErrorPolymorphism branch from 76a4e44 to 6278816 Compare June 10, 2025 05:51
Comment on lines +32 to +35
pub fn[T] FixedArray::each(
self : FixedArray[T],
f : (T) -> Unit?Error
) -> Unit?Error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one uses intrinsics, so it can't be modified like this directly. @Yu-zh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I remove my changes?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will remove the intrinsic for now.

@bobzhang
Copy link
Contributor

can you do a rebase? there is a rebase conflict

@Asterless
Copy link
Contributor Author

can you do a rebase? there is a rebase conflict

There are too many problems that rebase needs to solve. Maybe creating a new branch would be better🥲

@peter-jerry-ye
Copy link
Collaborator

can you do a rebase? there is a rebase conflict

There are too many problems that rebase needs to solve. Maybe creating a new branch would be better🥲

You need to improve your github workflow.

@peter-jerry-ye peter-jerry-ye force-pushed the Refactor/@Array-ErrorPolymorphism branch from eb5c125 to c45b350 Compare June 11, 2025 03:43
@peter-jerry-ye peter-jerry-ye enabled auto-merge (rebase) June 11, 2025 03:46
@peter-jerry-ye peter-jerry-ye merged commit badb104 into moonbitlang:main Jun 11, 2025
11 of 12 checks passed
@Asterless
Copy link
Contributor Author

can you do a rebase? there is a rebase conflict

There are too many problems that rebase needs to solve. Maybe creating a new branch would be better🥲

You need to improve your github workflow.

Thank you for your reminder! Actually, I have already realized this problem and won't make the same mistake again next time

@Asterless Asterless deleted the Refactor/@Array-ErrorPolymorphism branch June 12, 2025 19:35
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.

4 participants