Add partitionResults with single-pass destructuring#359
Conversation
Address review feedback from demystifyfp#358: replace two-pass Array.choose implementations with single-pass alternatives. Array uses ResizeArray, List uses tail-recursive loop, Seq returns seq instead of array. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| /// </summary> | ||
| /// <param name="xs">The input sequence of results.</param> | ||
| /// <returns>A tuple where the first element is an array of all Ok values and the second is an array of all Error values.</returns> | ||
| let partitionResults (xs: SeqNull<Result<'ok, 'error>>) : 'ok seq * 'error seq = |
There was a problem hiding this comment.
there is prior art in this repo for the Seq variant yielding arrays. There is no real benefit to downcasting here as it's hard to conceive of a different internal impl that beats accumulating and materializing
So my suggestion here would be for Seq.X to delegate to and Array.X to share this impl. On the array side, let the inputs be Arrays so it could be optimized to e.g. consider the input lengths. On the Seq side you can't but returning arrays lets people pattern match and/or lenght check the arrays immediately vs pretending its lazy and/or them upcasting again
There was a problem hiding this comment.
found it: #310
one notable thing is that ArrayCollector can be useful in this space but in the context of this lib stick with ResizeArray so it can be supported in the Fable impl (which did not have ArrayCollector at least at that time)
TheAngryByrd
left a comment
TheAngryByrd
left a comment
There was a problem hiding this comment.
Thanks for this! Looks good with two outstanding things:
- Couple nitpicks for XML docs
- Add corresponding docs over in https://github.com/demystifyfp/FsToolkit.ErrorHandling/tree/master/gitbook
(also thanks for all the review work @bartelink!)
| * [sequenceResultM](seq/sequenceResultM.md) | ||
| * [traverseResultA](seq/traverseResultA.md) | ||
| * [sequenceResultA](seq/sequenceResultA.md) | ||
| * [partitionResults](seq/partitionResults.md) |
There was a problem hiding this comment.
don't see array ref'd here - seed a new section if required...
There was a problem hiding this comment.
Yeah, we could do that, but it is already just repeats of the same docs for lists and seq where an array section simply would be another permutation.
As a user of the library, I have never 'missed' the array documentation. But we can add it in this commit if @TheAngryByrd would find it useful?
There was a problem hiding this comment.
Yeah create a new section please. Don't have to backfill anything that's missing, just reference anything that already exists.
TheAngryByrd
left a comment
TheAngryByrd
left a comment
There was a problem hiding this comment.
Thank you for the work!
3 participants
Proposed Changes
Adds
partitionResultsfunctions toArray,List, andSeqmodules that partition a collection ofResult<'ok, 'error>into a tuple of unwrapped ok values and error values.Closes #358
Implementation
All implementations use single-pass traversal as suggested in the issue discussion:
ResizeArrayaccumulators (per @njlr's suggestion)ResizeArrayaccumulators, returnsseqrather thanarray