feat: either.Reduce #4

Open

alexandradeas wants to merge 1 commit from feat/reduce_either into main

pull from: feat/reduce_either

merge into: mog:main

mog:main

mog:feat/either_map

Conversation 1 Commits 1 Files changed 2 +30

alexandradeas commented 2026-04-28 13:00:14 +00:00

Owner

Copy link

Adds the function Reduce to the either module that will reduce an either
value in a single value

Adds the function Reduce to the either module that will reduce an either value in a single value

alexandradeas added 1 commit 2026-04-28 13:00:14 +00:00

feat: either.Reduce ... 0e79dacfb0

Adds the function Reduce to the either module that will reduce an either
value in a single value

alexandradeas force-pushed feat/reduce_either from 0e79dacfb0 to 4aa72f5f98 2026-04-29 14:22:57 +00:00 Compare

alexandradeas force-pushed feat/reduce_either from 4aa72f5f98 to ddaba1b603 2026-04-29 14:27:37 +00:00 Compare

forgejo-actions commented 2026-04-29 14:30:30 +00:00

Copy link

AI Code Review

📋 MR Summary

Adds either.Reduce to collapse an Either into a single value and a TestReduce unit test. The implementation returns a function that dispatches to onLeft/onRight based on e.IsLeft(). Tests attempt to cover both Left and Right cases.

• Key Changes:
  • Added Reduce function to either/either.go
  • Added TestReduce to either/either_test.go
• Impact: either, either tests
• Risk Level: 🟡 Medium - New public API (Reduce) and unit test added. The Reduce implementation is simple but lacks defensive checks for nil handlers; the test contains at least one correctness bug that will cause the test suite to fail to compile/run.

Detailed Code Review

The Reduce implementation is straightforward and correctly dispatches to the provided handlers based on IsLeft(). However, there are two classes of issues: (1) the tests contain a loop/iteration bug that will make the test invalid (it ranges incorrectly over a slice and will iterate indices, not elements), and (2) the implementation does not validate that the provided handler functions are non-nil which will cause runtime panics if callers pass nil. Additionally, the test's invocation of Right has inconsistent generics compared to the Left invocation and may not compile depending on the function signature of Right in the codebase. Address the failing test loop first (it prevents running tests), then add defensive checks or document the panicking behavior of Reduce and add tests for that behavior.

📂 File Reviews

📄 `either/either.go` - Introduces Reduce that maps an Either[L,R] to a single T using provided handlers.

• Major [Bug]: Reduce calls the provided onLeft/onRight function pointers without validating they are non-nil. Calling Reduce(nil, onRight) or Reduce(onLeft, nil) will cause a runtime panic when the returned function is invoked.
  • Suggestion: Validate that onLeft and onRight are non-nil at construction time and return a clearer panic message, or decide to document that nil handlers are disallowed. Failing fast with a clear panic message is preferable to an obscure nil-function-call panic at runtime.

func Reduce[L any, R any, T any](onLeft func(L) T, onRight func(R) T) func(Either[L, R]) T {
    if onLeft == nil || onRight == nil {
        panic("either.Reduce: onLeft and onRight must be non-nil")
    }
    return func(e Either[L, R]) T {
        if e.IsLeft() {
            return onLeft(e.l)
        }
        return onRight(e.r)
    }
}

• Minor [Architecture]: Reduce accesses the Either's internal fields e.l and e.r directly. This couples Reduce to the internal representation of Either.
  • Suggestion: If the project exposes accessors or a Match/Unwrap-like API on Either, prefer using those to avoid coupling to unexported fields. If not available, leaving as-is is acceptable since Reduce is in the same package, but consider adding accessors/match to centralize access logic.

📄 `either/either_test.go` - Adds TestReduce to verify Reduce on Left and Right values.

• Critical [Bug]: The test iterates incorrectly: 'for c := range slices.Values(...)' iterates over indices (int) of the returned slice, not the slice elements. Using c.in will fail to compile/type-check because c is an int, not the struct element.
  • Suggestion: Use the two-value form to iterate over slice elements: 'for _, c := range slices.Values(...)' so c is the element (struct) with fields name,in,expect.

for _, c := range slices.Values([]struct {
    name   string
    in     Either[int, string]
    expect string
}{
    {name: "left", in: Left[int, string](10), expect: "10"},
    {name: "right", in: Right[int, string]("foo"), expect: "foo"},
}) {
    out := Reduce(func(i int) string { return fmt.Sprint(i) }, func(s string) string { return s })(c.in)
    assert.Equal(t, c.expect, out)
}

• Major [Bug]: In the test data, Left is constructed as Leftint, string but Right is constructed as Rightint. These generic parameter usages are inconsistent and may not match the declared signature of Right in the codebase, causing a compilation error.
  • Suggestion: Make the Right construction consistent with Left. If Right is declared as Right[L any, R any](r R) Either[L,R], call Rightint, string. If Right is declared with a single type parameter for the right side, use Rightstring. Verify the Right function signature in the project and make the test match it.

// If Right has two type params (LeftType, RightType):
{name: "right", in: Right[int, string]("foo"), expect: "foo"},

// If Right uses single type param for R only:
{name: "right", in: Right[string]("foo"), expect: "foo"},

• Minor [Testing]: There is no test coverage for the behaviour when nil handlers are passed to Reduce. Depending on the intended contract (panic vs allowed nil), this should be tested.
  • Suggestion: Add tests asserting the function panics when handlers are nil, or document that nil handlers are disallowed. Use assert.Panics or t.Fatalf accordingly.

assert.Panics(t, func() {
    _ = Reduce[int, string, string](nil, func(s string) string { return s })(Left[int, string](1))
})
// And similarly for a nil onRight

• Minor [Suggestion]: The test uses slices.Values which is a Go 1.21 addition (slices package). Ensure the project's minimum Go version supports this; otherwise the test will not compile in older toolchains.
  • Suggestion: If the project supports < Go 1.21, replace with a local literal slice and iterate over it normally.

cases := []struct{ name string; in Either[int,string]; expect string }{ /* ... */ }
for _, c := range cases { /* ... */ }

✅ Summary

• Overall Assessment: Critical test bug (incorrect range usage) and a potential compile error with Right's type parameters must be fixed before merge. Additionally, add defensive validation or documentation for nil handler behavior in Reduce.
• Priority Issues:
  • Fix the loop in TestReduce: use 'for _, c := range ...' so test iterates elements (currently iterates indices and will fail to compile).
  • Correct the generic invocation of Right in the test so it matches the project's Right signature (e.g., Rightint, string or Rightstring).
  • Decide and enforce behavior for nil handler functions in Reduce (either validate and panic with a clear message or document/handle nil).
• Minor Suggestions:
  • Consider exposing or using either.Match / accessor methods instead of accessing e.l and e.r directly to reduce coupling to internal fields.
  • Add tests for nil handler behavior (assert panics or document the behavior).
  • Document Reduce in package comments with its expected semantics (panic on nil handlers, whether it accepts pointer receivers, etc.).
  • Consider providing a method form (e.Reduce(onLeft,onRight)) on Either for discoverability; keep the functional variant if desired for composability.

---

🤖 AI Code Review | Generated with ai-code-review | Model: gpt-5-mini

⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.

## AI Code Review ### 📋 MR Summary Adds either.Reduce to collapse an Either into a single value and a TestReduce unit test. The implementation returns a function that dispatches to onLeft/onRight based on e.IsLeft(). Tests attempt to cover both Left and Right cases. - **Key Changes:** - Added Reduce function to either/either.go - Added TestReduce to either/either_test.go - **Impact:** either, either tests - **Risk Level:** 🟡 Medium - New public API (Reduce) and unit test added. The Reduce implementation is simple but lacks defensive checks for nil handlers; the test contains at least one correctness bug that will cause the test suite to fail to compile/run. ### Detailed Code Review The Reduce implementation is straightforward and correctly dispatches to the provided handlers based on IsLeft(). However, there are two classes of issues: (1) the tests contain a loop/iteration bug that will make the test invalid (it ranges incorrectly over a slice and will iterate indices, not elements), and (2) the implementation does not validate that the provided handler functions are non-nil which will cause runtime panics if callers pass nil. Additionally, the test's invocation of Right has inconsistent generics compared to the Left invocation and may not compile depending on the function signature of Right in the codebase. Address the failing test loop first (it prevents running tests), then add defensive checks or document the panicking behavior of Reduce and add tests for that behavior. #### 📂 File Reviews <details> <summary><strong>📄 `either/either.go`</strong> - Introduces Reduce that maps an Either[L,R] to a single T using provided handlers.</summary> - **Major** [Bug]: Reduce calls the provided onLeft/onRight function pointers without validating they are non-nil. Calling Reduce(nil, onRight) or Reduce(onLeft, nil) will cause a runtime panic when the returned function is invoked. - *Suggestion:* Validate that onLeft and onRight are non-nil at construction time and return a clearer panic message, or decide to document that nil handlers are disallowed. Failing fast with a clear panic message is preferable to an obscure nil-function-call panic at runtime. ```` func Reduce[L any, R any, T any](onLeft func(L) T, onRight func(R) T) func(Either[L, R]) T { if onLeft == nil || onRight == nil { panic("either.Reduce: onLeft and onRight must be non-nil") } return func(e Either[L, R]) T { if e.IsLeft() { return onLeft(e.l) } return onRight(e.r) } } ```` - **Minor** [Architecture]: Reduce accesses the Either's internal fields e.l and e.r directly. This couples Reduce to the internal representation of Either. - *Suggestion:* If the project exposes accessors or a Match/Unwrap-like API on Either, prefer using those to avoid coupling to unexported fields. If not available, leaving as-is is acceptable since Reduce is in the same package, but consider adding accessors/match to centralize access logic. </details> <details> <summary><strong>📄 `either/either_test.go`</strong> - Adds TestReduce to verify Reduce on Left and Right values.</summary> - **Critical** [Bug]: The test iterates incorrectly: 'for c := range slices.Values(...)' iterates over indices (int) of the returned slice, not the slice elements. Using c.in will fail to compile/type-check because c is an int, not the struct element. - *Suggestion:* Use the two-value form to iterate over slice elements: 'for _, c := range slices.Values(...)' so c is the element (struct) with fields name,in,expect. ```` for _, c := range slices.Values([]struct { name string in Either[int, string] expect string }{ {name: "left", in: Left[int, string](10), expect: "10"}, {name: "right", in: Right[int, string]("foo"), expect: "foo"}, }) { out := Reduce(func(i int) string { return fmt.Sprint(i) }, func(s string) string { return s })(c.in) assert.Equal(t, c.expect, out) } ```` - **Major** [Bug]: In the test data, Left is constructed as Left[int, string](10) but Right is constructed as Right[int]("foo"). These generic parameter usages are inconsistent and may not match the declared signature of Right in the codebase, causing a compilation error. - *Suggestion:* Make the Right construction consistent with Left. If Right is declared as Right[L any, R any](r R) Either[L,R], call Right[int, string]("foo"). If Right is declared with a single type parameter for the right side, use Right[string]("foo"). Verify the Right function signature in the project and make the test match it. ```` // If Right has two type params (LeftType, RightType): {name: "right", in: Right[int, string]("foo"), expect: "foo"}, // If Right uses single type param for R only: {name: "right", in: Right[string]("foo"), expect: "foo"}, ```` - **Minor** [Testing]: There is no test coverage for the behaviour when nil handlers are passed to Reduce. Depending on the intended contract (panic vs allowed nil), this should be tested. - *Suggestion:* Add tests asserting the function panics when handlers are nil, or document that nil handlers are disallowed. Use assert.Panics or t.Fatalf accordingly. ```` assert.Panics(t, func() { _ = Reduce[int, string, string](nil, func(s string) string { return s })(Left[int, string](1)) }) // And similarly for a nil onRight ```` - **Minor** [Suggestion]: The test uses slices.Values which is a Go 1.21 addition (slices package). Ensure the project's minimum Go version supports this; otherwise the test will not compile in older toolchains. - *Suggestion:* If the project supports < Go 1.21, replace with a local literal slice and iterate over it normally. ```` cases := []struct{ name string; in Either[int,string]; expect string }{ /* ... */ } for _, c := range cases { /* ... */ } ```` </details> ### ✅ Summary - **Overall Assessment:** Critical test bug (incorrect range usage) and a potential compile error with Right's type parameters must be fixed before merge. Additionally, add defensive validation or documentation for nil handler behavior in Reduce. - **Priority Issues:** - Fix the loop in TestReduce: use 'for _, c := range ...' so test iterates elements (currently iterates indices and will fail to compile). - Correct the generic invocation of Right in the test so it matches the project's Right signature (e.g., Right[int, string]("foo") or Right[string]("foo")). - Decide and enforce behavior for nil handler functions in Reduce (either validate and panic with a clear message or document/handle nil). - **Minor Suggestions:** - Consider exposing or using either.Match / accessor methods instead of accessing e.l and e.r directly to reduce coupling to internal fields. - Add tests for nil handler behavior (assert panics or document the behavior). - Document Reduce in package comments with its expected semantics (panic on nil handlers, whether it accepts pointer receivers, etc.). - Consider providing a method form (e.Reduce(onLeft,onRight)) on Either for discoverability; keep the functional variant if desired for composability. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gpt-5-mini` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*

All checks were successful

Hide all checks

PR / ai-review (pull_request) Successful in 2m54s

Details

PR / build (pull_request) Successful in 30s

Details

This pull request has changes conflicting with the target branch.

• either/either_test.go

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin feat/reduce_either:feat/reduce_either

git switch feat/reduce_either

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

mog/goz!4

No description provided.