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

Add TaskSeq.concat overloads for seq, list, array, resizearray #237

Merged
merged 1 commit into from
Mar 16, 2024

Conversation

abelbraaksma
Copy link
Member

Part of the push for good coverage of surface area functions, see #208 for an overview. This implements TaskSeq.concat overloads for seq, list, array and ResizeArray. Reasoning behind specific overloads is that multiple overloads with flex types is not possible: dotnet/fsharp#4943.

Each of these behave exactly like their Seq counterparts:

  • raises on null source
  • raises normal NullReferenceException when a nested sequence returns null

As before, the xml doc blibs have been taken from seq.fs and modified a bit for readability and applicability to TaskSeq.

The signatures are as follows:

static member concat: sources: TaskSeq<#TaskSeq<'T>> -> TaskSeq<'T> // already there
static member concat: sources: TaskSeq<'T seq> -> TaskSeq<'T>
static member concat: sources: TaskSeq<'T[]> -> TaskSeq<'T>
static member concat: sources: TaskSeq<'T list> -> TaskSeq<'T>
static member concat: sources: TaskSeq<ResizeArray<'T>> -> TaskSeq<'T>

TODO list:

  • implement TaskSeq.concat for seq
  • implement TaskSeq.concat for array
  • implement TaskSeq.concat for list
  • implement TaskSeq.concat for ResizeArray

@abelbraaksma abelbraaksma merged commit 2fc7d94 into main Mar 16, 2024
6 checks passed
@abelbraaksma abelbraaksma deleted the implement-concat-overloads branch March 16, 2024 14:52
Copy link
Member

@bartelink bartelink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - nits and musings added after the fact as usual, sorry!

@@ -215,14 +215,55 @@ type TaskSeq =

/// <summary>
/// Combines the given task sequence of task sequences and concatenates them end-to-end, to form a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the given task sequence of task sequences" feels like a mouthful, and Combines does not really help (Flattens would be a great one word TL;DR intro to my mind if I didnt know what concat did)
If I was writing something random from scratch:

Yields a task sequence that unrolls each task sequence emanating from the source, concatenating them end to end,

/// <exception cref="T:ArgumentNullException">Thrown when the input task sequence of task sequences is null.</exception>
static member concat: sources: TaskSeq<#TaskSeq<'T>> -> TaskSeq<'T>

/// <summary>
/// Combines the given task sequence of sequences and concatenates them end-to-end, to form a
/// new flattened, single task sequence.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also these are Combinators, so I guess they do Combine in that technical sense (which we all know is the best sense!)

Everything is always "new" for all TaskSeq operators (hence my campaign for a keyword like yield conveying the fact that stuff comes in, has treatments applied, and flows out, remaining lazy)
flattened is great

I'm not sure 'new' is a useful word; if there is a 'Combines' in the description, then there is something new happening.
But we are not actually making a thing that is 'new', it's ephemeral

Hence mq quest for a workd like 'renders' or 'yields' to convey that while we are combining things, there is not really anything 'new' except in the sense that any function that does not return its input results in a 'new' value.

Again, just musing...

///
/// <param name="sources">The input task sequence of sequences.</param>
/// <returns>The resulting, flattened task sequence.</returns>
/// <exception cref="T:ArgumentNullException">Thrown when the input task sequence of task sequences is null.</exception>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess Seq doesnt either but I'd like to see mention of NRE happening if there is a null in the input
esp for arrays.
But then LINQ SelectMany also does not document it and behaves the same way
So I guess that's case closed based on prior art
(I can recall writing test snippets in 2007 because of this not being written anywhere I was aware of)

But kudos for having the test in the first instance so it's all good I guess

@abelbraaksma abelbraaksma added this to the v0.4.0 milestone Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants