Skip to content
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.

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

feat(Seq): Add sequenceResultA #254

Open
bartelink opened this issue Feb 14, 2024 · 6 comments
Open

feat(Seq): Add sequenceResultA #254

bartelink opened this issue Feb 14, 2024 · 6 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@bartelink
Copy link
Contributor

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 TheAngryByrd added enhancement New feature or request good first issue Good for newcomers labels Feb 14, 2024
@TheAngryByrd
Copy link
Collaborator

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

@bartelink
Copy link
Contributor Author

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:

  • sequenceResultM: Result<'t, 'e> seq -> Result<'t[], 'e>
  • sequenceResultA: Result<'t, 'e> seq -> Result<'t[], 'e[]>
    that would align with the List module's signatures/design

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
Copy link
Collaborator

TheAngryByrd commented Feb 14, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

bartelink commented Feb 15, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants