fp-go Pull Request Review

// ❌ WRONG - v1 import
import "github.com/IBM/fp-go/option"

// ✅ CORRECT - v2 import
import O "github.com/IBM/fp-go/v2/option"

// ❌ WRONG - data-first
option.Map(myOption, transformFunc)

// ✅ CORRECT - data-last
option.Map(transformFunc)(myOption)

// ✅ CORRECT - in pipeline
F.Pipe2(
    myOption,
    O.Map(transformFunc),
    O.GetOrElse(LZ.Of("default")),
)

// ❌ AVOID - unnecessary lambda wrapping
pipeline := F.Flow2(
    func(s string) O.Option[string] { return O.FromPredicate(S.IsNonEmpty)(s) },
    func(o O.Option[string]) string { return O.GetOrElse(func() string { return "" })(o) },
)

// ✅ CORRECT - point-free composition
pipeline := F.Flow2(
    O.FromPredicate(S.IsNonEmpty),   // string -> Option[string]
    O.GetOrElse(LZ.Of("")),          // Option[string] -> string; LZ = v2/lazy
)

// ❌ AVOID - inline comparison
A.Filter(func(x int) bool { return x > 18 })

// ✅ CORRECT - use numeric combinator
A.Filter(N.MoreThan(18))

// ❌ AVOID - field-access lambda inside a pipeline step
RIO.Map(func(u User) string { return u.Name })

// ✅ CORRECT - named leaf accessor or lens getter
RIO.Map(nameLens.Get)

// ❌ AVOID - lambda that only threads its argument into a pipeline
RIO.Chain(func(u User) RIO.ReaderIOResult[[]Order] {
    return F.Pipe1(fetchOrders(u.ID), RIO.LogEntryExit[[]Order]("fetchOrders"))
})

// ✅ CORRECT - compose the steps
RIO.Chain(F.Flow3(getUserID, fetchOrders, RIO.LogEntryExit[[]Order]("fetchOrders")))

// ❌ AVOID - hand-written Reader/IO closures around a Go function
func fetchUser(id int) RIO.ReaderIOResult[User] {
    return func(ctx context.Context) func() R.Result[User] {
        return func() R.Result[User] { return R.TryCatchError(repo.FindUser(ctx, id)) }
    }
}

// ✅ CORRECT - lift it
fetchUser := RIO.Eitherize1(repo.FindUser) // repo.FindUser: func(context.Context, int) (User, error)

// ❌ AVOID - Either with error
func fetchData() E.Either[error, Data] { ... }

// ✅ CORRECT - use Result
func fetchData() R.Result[Data] { ... }

// ✅ CORRECT - Either with custom error type
func validate() E.Either[ValidationError, Data] { ... }

// ❌ WRONG - forgot to execute
result := readConfig("config.json")  // returns IO[Config], not Config

// ✅ CORRECT - execute with ()
result := readConfig("config.json")()

// ❌ WRONG - in ReaderIOResult, forgot inner ()
res := pipeline(ctx)         // returns func() Result[A], nothing has run yet

// ✅ CORRECT - execute both context and IO
res := pipeline(ctx)()       // Result[A] — ONE value

// ❌ WRONG - Result[A] is a single value, not a (value, error) tuple
value, err := pipeline(ctx)()

// ✅ CORRECT - unwrap to idiomatic Go at the boundary
value, err := R.Unwrap(pipeline(ctx)())

// ❌ AVOID - using ReaderIOResult for pure computation
// (also note: in context/readerioresult the context is baked in —
//  it is RIO.Of[A] and RIO.Map[A, B], with no environment type parameter)
func processUsers(users []User) RIO.ReaderIOResult[string] {
    return F.Pipe1(
        RIO.Of(users),
        RIO.Map(pureTransform),
    )
}

// ✅ CORRECT - pure computation, no monad needed
func processUsers() func([]User) string {
    return F.Flow2(
        A.FilterMap(toAdultName()),
        A.Intercalate(S.Monoid)(","),
    )
}

// ❌ AVOID - stuffing deps into context.Context
func fetchUser(id int) RIO.ReaderIOResult[User] {
    return func(ctx context.Context) func() R.Result[User] {
        db := ctx.Value("db").(DBClient)  // runtime type assertion
        // ...
    }
}

// ✅ CORRECT - typed dependencies with Effect
type Deps struct {
    DB     DBClient
    Logger Logger
}

// Lift an idiomatic function that receives the deps and the context.
// queryUser is func(Deps, context.Context, int) (User, error); deps.DB is compile-time checked.
func fetchUser() EF.Kleisli[Deps, int, User] {
    return EF.Eitherize1(queryUser)
}

// ❌ AVOID - manual error handling
func parseNumber(s string) R.Result[int] {
    n, err := strconv.Atoi(s)
    if err != nil {
        return R.Left[int](err)   // Left[A] — A is the success type
    }
    return R.Of(n)                // NOT R.Right[error](n); Right[A any](v A)
}

// ✅ CORRECT - use Eitherize
var parseNumber = R.Eitherize1(strconv.Atoi)

// ✅ CORRECT - in pipeline
pipeline := F.Flow2(
    R.Eitherize1(strconv.Atoi),
    R.Map(N.Mul(2)),
)

// ❌ AVOID - manual setter functions
func setUser(u User) func(State) State {
    return func(s State) State { s.User = u; return s }
}

pipeline := F.Pipe1(
    RIO.Do(State{}),
    RIO.Bind(setUser, fetchUser),
)

// ✅ CORRECT - use lens
var userLens = L.MakeLens(
    func(s State) User { return s.User },
    func(s State, u User) State { s.User = u; return s },
)

pipeline := F.Pipe1(
    RIO.Do(State{}),
    RIO.Bind(userLens.Set, fetchUser),
)

// ✅ EVEN BETTER - use code generation
//go:generate go run github.com/IBM/fp-go/v2 lens --dir . --filename gen_lens.go

// fp-go:Lens
type State struct {
    User User
}

// Then use generated lens
lenses := MakeStateLenses()
pipeline := F.Pipe1(
    RIO.Do(State{}),
    RIO.Bind(lenses.User.Set, fetchUser),
)

// ❌ WRONG - using Bind when steps are independent
pipeline := F.Pipe2(
    RIO.Do(Summary{}),
    RIO.Bind(userLens.Set, func(_ Summary) RIO.ReaderIOResult[User] {
        return fetchUser(42)  // doesn't use state
    }),
    RIO.Bind(weatherLens.Set, func(_ Summary) RIO.ReaderIOResult[Weather] {
        return fetchWeather("NYC")  // doesn't use state
    }),
)

// ✅ CORRECT - use ApS for independent steps
pipeline := F.Pipe2(
    RIO.Do(Summary{}),
    RIO.ApS(userLens.Set, fetchUser(42)),
    RIO.ApS(weatherLens.Set, fetchWeather("NYC")),
)

// ✅ CORRECT - ApS for the independent first step, Bind for the dependent one
pipeline := F.Pipe2(
    RIO.Do(Pipeline{}),
    RIO.ApS(userLens.Set, fetchUser(42)),
    RIO.Bind(configLens.Set, F.Flow2(userLens.Get, fetchConfigForUser)),
)

// ❌ AVOID - manual loop with error handling
func fetchAll(ids []int) RIO.ReaderIOResult[[]User] {
    return func(ctx context.Context) func() R.Result[[]User] {
        return func() R.Result[[]User] {
            users := make([]User, 0, len(ids))
            for _, id := range ids {
                user, err := R.Unwrap(fetchUser(id)(ctx)())
                if err != nil {
                    return R.Left[[]User](err)
                }
                users = append(users, user)
            }
            return R.Of(users)
        }
    }
}

// ✅ CORRECT - use TraverseArray, point-free (return the Kleisli, don't take ids)
func fetchAll() RIO.Kleisli[[]int, []User] {
    return RIO.TraverseArray(fetchUser)
}

// ❌ AVOID - breaking the pipeline for logging
pipeline := F.Pipe1(
    fetchUser(42),
    RIO.Chain(func(user User) RIO.ReaderIOResult[User] {
        log.Printf("Fetched user: %v", user)
        return RIO.Of(user)
    }),
)

// ✅ CORRECT - structured logging with TapSLog (logs value or error)
pipeline := F.Pipe1(
    fetchUser(42),
    RIO.TapSLog[User]("User fetched"),
)

// ✅ CORRECT - printf-style logging with TapIOK
pipeline := F.Pipe1(
    fetchUser(42),
    RIO.TapIOK(IO.Logf[User]("Fetched user: %v")),
)

// ❌ WRONG - var is allocated even if never called
var processUser = F.Flow2(getName, strings.ToUpper)

// ✅ CORRECT - zero cost until called
func processUser() func(User) string {
    return F.Flow2(getName, strings.ToUpper)
}

// option / result / ioresult: Map[A, B](f func(A) B) — BOTH inferable from f
O.Map(toLength)              // ✅ preferred, no annotation at all
O.Map[string, int](toLength) // ✅ legal but redundant (note the order: A then B)

// Ap[B, A](fa M[A]) — B is not recoverable from fa, so it leads
O.Ap[int](fa)                // ✅

// either / reader / readerio*: the error or environment type leads
E.Map[error](f)              // ✅ either.Map[E, A, B]
RD.Map[context.Context](f)   // ✅ reader.Map[R, A, B]

// effect: C leads and is often not inferable
EF.Map[Deps](f)              // ✅
EF.Provide[string](deps)     // ✅ Provide[A, C] cannot infer A through its result

// ❌ AVOID - manual nested access
func getStreetName(p Person) string {
    if p.Address != nil && p.Address.Street != nil {
        return p.Address.Street.Name
    }
    return ""
}

// ✅ CORRECT - compose lenses
streetNameInPerson := F.Pipe2(
    personAddressLens,
    LO.Compose[Person, *Street](defaultAddress)(addressStreetLens),
    LO.ComposeOption[Person, string](defaultStreet)(streetNameLens),
)

name := streetNameInPerson.Get(person)  // Option[string]

// ❌ WRONG - closure mutates a captured slice
var acc []string
A.Map(func(u User) User {
    acc = append(acc, u.Name)  // hidden side effect
    return u
})

// ✅ CORRECT - derive a new value, no captured mutation
names := F.Pipe1(users, A.Map(getName))

// ❌ WRONG - lens setter mutates a shared slice in place
//   append may reuse the original backing array (shallow struct copy)
func(u User, t []string) User { u.Tags = append(u.Tags, t...); return u }

// ✅ CORRECT - assign a freshly built value
func(u User, t []string) User { u.Tags = t; return u }

// ❌ WRONG - panics if the key is missing or has another type; plain string key
getUser := RIO.FromReader(func(ctx context.Context) string {
    return ctx.Value("user").(string)
})

// ✅ CORRECT - typed key, Option result, caller decides what "missing" means
type ctxKey string
const userKey ctxKey = "user"
getUser := F.Pipe1(RIO.AskValue[string](userKey), RIO.Map(O.GetOrElse(LZ.Of("anonymous"))))

// ❌ WRONG - hand-derived context; cancel discarded -> leaked timer
RIO.Local[A](func(ctx context.Context) ContextCancel {
    tctx, _ := context.WithTimeout(ctx, 5*time.Second)
    return pair.MakePair(func() {}, tctx)
})

// ❌ WRONG - scoping done outside the pipeline, by hand
ctx, cancel := context.WithTimeout(context.WithValue(ctx, userKey, u), 5*time.Second)
defer cancel()
res := pipeline(ctx)()

// ✅ CORRECT - scoping as operators; cancel always released
res := F.Pipe2(
    pipeline,
    RIO.WithTimeout[A](5*time.Second),
    RIO.WithValue[A](userKey, u),
)(ctx)()

// ❌ WRONG - Unpack + defer just to install a logger
cancel, lctx := pair.Unpack(logging.WithLogger(l)(ctx)); defer cancel()

// ✅ CORRECT - WithLogger already has Local's shape
F.Pipe1(pipeline, RIO.Local[A](logging.WithLogger(l)))