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

[API Proposal]: Add Enumerable.Concat & Flatten overloads #54220

Open
TonyValenti opened this issue Jun 15, 2021 · 23 comments
Open

[API Proposal]: Add Enumerable.Concat & Flatten overloads #54220

TonyValenti opened this issue Jun 15, 2021 · 23 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Linq
Milestone

Comments

@TonyValenti
Copy link

TonyValenti commented Jun 15, 2021

Background and Motivation

When working with different APIs, often times it increases code readability to aggregate results and then linearize them. For example, I could write the code:

Command.NameInfo.OnlineAccounts = new List<OnlineAccount>();
Command.NameInfo.OnlineAccounts.AddRange(Input.WebAddresses.Select(x => CreateWebAddress(x)));
Command.NameInfo.OnlineAccounts.AddRange(Input.EmailAddresses.Select(x => CreateEmailAddress(x)));

or I could write:

Command.NameInfo.OnlineAccounts = new[] {
    Input.WebAddresses.Select(x => CreateWebAddress(x)),
    Input.EmailAddresses.Select(x => CreateEmailAddress(x))
}.Concat().ToList();

The second is much more clear and concise.

Proposed API

namespace System.Linq 
{
    public static class Enumerable 
    {
        public static IEnumerable<T> Concat<T>(this IEnumerable<TSource> first, IEnumerable<TSource> second);
+        public static IEnumerable<T> Concat<T>(params IEnumerable<T>[] sources);
+        public static IEnumerable<T> Concat<T>(this IEnumerable<IEnumerable<T>> sources);
    }
}

Usage Examples

var source = Enumerable.Range(0, 2);

Enumerable.Concat(source, source, source, source); // 0, 1, 0, 1, 0, 1, 0, 1
Enumerable.Repeat(source, 3).Concat(); // 0, 1, 0, 1, 0, 1

Alternative Designs

Just use SelectMany(x => x)

Risks

None other than System.Linq size concerns.

@TonyValenti TonyValenti added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 15, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Linq untriaged New issue has not been triaged by the area owner labels Jun 15, 2021
@ghost
Copy link

ghost commented Jun 15, 2021

Tagging subscribers to this area: @eiriktsarpalis
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

When working with different APIs, often times it increases code readability to aggregate results and then linearize them. For example, I could write the code:

Command.NameInfo.OnlineAccounts = new List<OnlineAccount>();
Command.NameInfo.OnlineAccounts.AddRange(Input.WebAddresses.Select(x => CreateWebAddress(x)));
Command.NameInfo.OnlineAccounts.AddRange(Input.EmailAddresses.Select(x => CreateEmailAddress(x)));

or I could write:

            Command.NameInfo.OnlineAccounts = new[] {
                Input.WebAddresses.Select(x => CreateWebAddress(x)),
                Input.EmailAddresses.Select(x => CreateEmailAddress(x))
            }.SelectMany().ToList();

The second is much more clear and concise.

Proposed API

public static IEnumerable<T> SelectMany<T>(this IEnumerable<IEnumerable<T>> This) {
            return This.SelectMany(x => x);
        }

        public static IEnumerable<T> Concat<T>(this IEnumerable<IEnumerable<T>> This) {
            var ret = Enumerable.Empty<T>();

            foreach (var item in This) {
                if(item == null) {
                    throw new NullReferenceException();
                }

                if(ret == Enumerable.Empty<T>()) {
                    ret = item;
                } else {
                    ret = Enumerable.Concat(ret, item);
                }

            }

            return ret;
        }

Usage Examples

            Command.NameInfo.OnlineAccounts = new[] {
                Input.WebAddresses.Select(x => CreateWebAddress(x)),
                Input.EmailAddresses.Select(x => CreateEmailAddress(x))
            }.SelectMany().ToList();

Alternative Designs

Just use SelectMany(x => x)

Risks

None

Author: TonyValenti
Assignees: -
Labels:

api-suggestion, area-System.Linq, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

I'm assuming SelectMany() and Concat() are just alternative names for the same proposed method? Why not just stick with one? I personally would prefer exposing Concat() as an overload, since F# uses the same name for its equivalent method.

@eiriktsarpalis eiriktsarpalis added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Jun 15, 2021
@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Jun 15, 2021
@svick
Copy link
Contributor

svick commented Jun 16, 2021

Note that another way to write this code is to use the existing overload of Concat:

Command.NameInfo.OnlineAccounts = 
    Input.WebAddresses.Select(x => CreateWebAddress(x))
        .Concat(Input.EmailAddresses.Select(x => CreateEmailAddress(x)))
        .ToList();

Or:

Command.NameInfo.OnlineAccounts = Enumerable.Concat(
    Input.WebAddresses.Select(x => CreateWebAddress(x)),
    Input.EmailAddresses.Select(x => CreateEmailAddress(x))
).ToList();

But you do have to choose between the fluent assymetric form and the non-fluent symmetric form. So I think the proposed overload of Concat would still be a good addition.

@eiriktsarpalis
Copy link
Member

Agreed, @TonyValenti if you could update the OP following the API proposal template we can consider this proposal for .NET 7.

@terrajobst terrajobst removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 25, 2021
@danielchalmers
Copy link
Contributor

I often make a Flatten<T>(this IEnumerable<T> l)=>l.SelectMany(x=>x) extension so I would love to see this in the BCL!

@eiriktsarpalis
Copy link
Member

@danielchalmers please consider upvoting the original post, it provides a good signal on whether a particular proposal is popular!

@eiriktsarpalis eiriktsarpalis added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Oct 14, 2021
@eiriktsarpalis
Copy link
Member

Hi @TonyValenti, before I can mark this ready for API review would it be possible to update the original post so that it follows our API review template?

@TonyValenti
Copy link
Author

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Oct 14, 2021

A few issues with the original text I would like to call out:

  • Proposal contains two different methods for the same functionality: SelectMany and Concat. One needs to be removed, I would recommend keeping Concat.
  • The proposed API is missing namespace and declaring class for the proposed methods.
  • The proposed API contains implementation, which is not needed from the API review perspective.

The API review document contains links to a few API proposal issues, if you're looking for real-world examples.

@TonyValenti
Copy link
Author

@eiriktsarpalis Thanks! I just updated it. I left the implementation just because I felt that added more clarity. I also added a params overload for Concat that will make joining multiple enumerables easier.

@eiriktsarpalis eiriktsarpalis changed the title SelectMany<T>() and Concat<T>() [API Proposal]: Add Enumerable.Concat overloads for variadic arguments nested enumerables Oct 15, 2021
@eiriktsarpalis eiriktsarpalis changed the title [API Proposal]: Add Enumerable.Concat overloads for variadic arguments nested enumerables [API Proposal]: Add Enumerable.Concat overloads for variadic arguments and nested enumerables Oct 15, 2021
@eiriktsarpalis
Copy link
Member

@TonyValenti I've updated your proposal slightly to better match the template. The reason we want this is to allow the API review committee quickly read and understand what is being proposed.

@eiriktsarpalis eiriktsarpalis self-assigned this Oct 15, 2021
@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Oct 15, 2021
@TonyValenti
Copy link
Author

Great! Thanks so much!

@bartonjs
Copy link
Member

bartonjs commented Oct 19, 2021

Video

  • We changed the signature of the params overload to Concat to support extension invocation and look like an easier overload.
  • We renamed the Concat(IEnumerable<IEnumerable<T>>) to Flatten to better describe its use.
namespace System.Linq 
{
    public static partial class Enumerable 
    {
        public static IEnumerable<TSource> Concat<TSource>(
            this IEnumerable<TSource> first,
             IEnumerable<TSource> second,
             params IEnumerable<TSource>[] rest);


        public static IEnumerable<TSource> Flatten<TSource>(this IEnumerable<IEnumerable<TSource>> sources);
    }

    public static partial class Queryable
    {
        public static IQueryable<TSource> Concat<TSource>(
            this IQueryable<TSource> source1,
             IEnumerable<TSource> source2,
             params IEnumerable<TSource>[] rest);


        public static IQueryable<TSource> Flatten<TSource>(this IQueryable<IEnumerable<TSource>> sources);
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Oct 19, 2021
@svick
Copy link
Contributor

svick commented Oct 19, 2021

@bartonjs Why does the params overload have the second parameter? Couldn't it be just:

public static IEnumerable<TSource> Concat<TSource>(
    this IEnumerable<TSource> first,
    params IEnumerable<TSource>[] rest);

@eiriktsarpalis
Copy link
Member

@svick we wanted to prevent concatenation with zero parameters, i.e. first.Concat(). Technically speaking it's a valid operation but it doesn't make a lot of sense.

@eiriktsarpalis
Copy link
Member

I just realized that I completely forgot about IQueryable overloads:

namespace System.Linq 
{
    public static partial class Queryable
    {
        public static IQueryable<TSource> Concat<TSource>(
            this IQueryable<TSource> source1,
             IEnumerable<TSource> source2,
             params IEnumerable<TSource>[] rest);


        public static IQueryable<TSource> Flatten<TSource>(this IQueryable<IEnumerable<TSource>> sources);
    }
}

@bartonjs should we be reviewing those seperately?

@stephentoub
Copy link
Member

Did we discuss params Span at all? Assuming that shows up in C# 11, would we still use params arrays here, would we add both, or would we say as a general rule we prefer spans moving forward?

@bartonjs
Copy link
Member

@bartonjs should we be reviewing those seperately?

My recollection is we have a blanket policy of "oh, right, make queryable look the same as enumerable"

@bartonjs
Copy link
Member

bartonjs commented Oct 19, 2021

Did we discuss params Span at all? Assuming that shows up in C# 11, would we still use params arrays here, would we add both, or would we say as a general rule we prefer spans moving forward?

We did. Partly it'll end up depending on what it looks like for those methods to be called by VB/F#. If it Just Works, then @terrajobst suggested we'd go back and look at new-in-7 API that we could remove the redundancies from. If it doesn't just work, then we'll probably add them in parallel anywhere other than low-level types.

@eiriktsarpalis eiriktsarpalis changed the title [API Proposal]: Add Enumerable.Concat overloads for variadic arguments and nested enumerables [API Proposal]: Add Enumerable.Concat & Flatten overloads Oct 21, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 4, 2021
@eiriktsarpalis eiriktsarpalis added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-approved API was approved in API review, it can be implemented labels Nov 15, 2021
@ghost
Copy link

ghost commented Nov 15, 2021

This issue has been marked with the api-needs-work label. This may suggest that the proposal requires further refinement before it can be considered for API review. Please refer to our API review guidelines for a detailed description of the process.

When ready to submit an amended proposal, please ensure that the original post in this issue has been updated, following the API proposal template and examples as provided in the guidelines.

@eiriktsarpalis
Copy link
Member

Changed to api-needs-work due to feedback in #61230. cc @stephentoub

@eiriktsarpalis eiriktsarpalis modified the milestones: 7.0.0, Future Nov 15, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 15, 2021
@TahirAhmadov
Copy link

@bartonjs did you consider the solution I proposed here ?

@LeaFrock
Copy link
Contributor

No progress for .NET 8? Does IQueryable block it?

@eiriktsarpalis eiriktsarpalis removed their assignment Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Linq
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants