demystifyfp / FsToolkit.ErrorHandling

An opinionated F# Library for error handling
https://demystifyfp.gitbook.io/fstoolkit-errorhandling
MIT License
462 stars 59 forks source link

feat(Seq): Add sequenceResultA #254

Open bartelink opened 6 months ago

bartelink commented 6 months ago

There's a List.sequenceResultA

I'd find use for a Seq.sequenceResultA: Result<'t, 'e> seq -> Result<'t[], 'e[]>

This would align with how Async.Parallel works (accumulate outputs in a ResizeArray, yield an array) for efficiency

Example desired consumption

let res: Result<EmailAddress[], string> =
    seq { for i in Seq.distinct inputAddrs -> EmailAddress.tryParse i |> Result.requireSome i }
    |> Seq.sequenceResultA
    |> Result.mapError (fun es -> $"""Invalid inputs: {es |> String.concat ","}""" )

Example current workaround:

let res: Result<EmailAddress list, string> =
    [ for i in Seq.distinct inputAddrs -> EmailAddress.tryParse i |> Result.requireSome i ]
    |> List.sequenceResultA
    |> Result.mapError (fun es -> $"""Invalid inputs: {es |> String.concat ","}""" )

(This is literally what I have, and I have limited awareness of the Validation modules etc so maybe they offer a better workaround, but it does seem that in general there's a hole in the completeness of the operations hence raising this)

TheAngryByrd commented 6 months ago

Yep I agree this is a good idea. Would you like to submit a pull request for it?

bartelink commented 6 months ago

I guess arm could be twisted

Some questions though.... sequenceResultM yields to Result<'t seq, 'err> sequenceResultA logically would yield Result<'t seq, 'err seq>

I find the semantics of the laziness of that hard to reason about, unless you can express it crisply? (I'm thinking it should not be lazy)

If it's not lazy, it needs to be materialized internally. If you're doing that, it's a bit daft not to just yield that given that the result is going to be :> seq<Result<_, _>

So I think the answer might be to make an Array module with:

Arguably if that was added, the Seq one could be obsoleted (unless it actually has well defined lazy semantics that is, but I doubt it)

TheAngryByrd commented 6 months ago

I find the semantics of the laziness of that hard to reason abou

Yeah it seems like it's a Seq.fold, so I don't think it's going to be something that can lazily be evaluated.

Seq one could be obsoleted

Not necessarily because there are other enumerables beyond F#'s Array/List in the BCL and it's handy for those.

bartelink commented 6 months ago

Updates from spiking:

  1. Fantomas default settings drive me wild (one pipe forcing a newline and other such nonsense). It's a real turnoff (and paket is IMO a bad idea for libraries). The ones TaskSeq are pretty tolerable. The ones in FSharp.AWS.DyamoDB, which are a fork with 3 or 4 tweaks from TaskSeq, are very good
  2. Do signatures need to be binary compatible? I'm going with putting it in the Seq module, and was going to make the sequenceResultM return an array on the basis that that's often useful, and it's what I intend to do for the A variant
  3. I'm thinking not to do an Array module, as realistically one would want to have most of the List stuff in there too (but if no signature changes can be tolerated, then that's a way of sidestepping that issue). Also I am interested in having it the impl be tolerant of seq inputs, which does not totally align with that one would expect from an Array module.
TheAngryByrd commented 6 months ago

one pipe forcing a newline and other such nonsense

I prefer this so this won't change.

Do signatures need to be binary compatible?

Yes unless you want to retarget for v5.

bartelink commented 6 months ago

I prefer this so this won't change.

Yeah, ultimately defaults matter so not a problem (and I do get that lots of people are happy with that layout)

Yes unless you want to retarget for v5.

I can make it API compatible in the context of this PR and prepare a follow-up PR to change the contract from seq to [] in a minimal way to be merged in the V5 timeframe

Also happy to leave the original impl exactly as it is; I have no doubt it's much closer to the idioms used in other impl pieces

~I'll assume the former~ let's discuss in the context of the PR as I'm flip-flopping, it's not time-sensitive for me, and you're the maintainer that has to deal with it all