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]: Enumerable.Order and OrderDescending #67194

Closed
terrajobst opened this issue Mar 26, 2022 · 29 comments · Fixed by #70525
Closed

[API Proposal]: Enumerable.Order and OrderDescending #67194

terrajobst opened this issue Mar 26, 2022 · 29 comments · Fixed by #70525
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Linq
Milestone

Comments

@terrajobst
Copy link
Member

terrajobst commented Mar 26, 2022

Background and motivation

Linq provides OrderBy and OrderByDescending which implies that I'm sorting based on some part of the T. Very often I find myself wanting to just sort by the T itself.

API Proposal

namespace System.Linq
{
    public partial class Enumerable
    {
        public static IOrderedEnumerable<T> Order<T>(this IEnumerable<T> source) where T: IComparable<T>;
        public static IOrderedEnumerable<T> Order<T>(this IEnumerable<T> source, IComparer<T> comparer);

        public static IOrderedEnumerable<T> OrderDescending<T>(this IEnumerable<T> source) where T: IComparable<T>;
        public static IOrderedEnumerable<T> OrderDescending<T>(this IEnumerable<T> source, IComparer<T> comparer);
    }
}

API Usage

var data = new[] { 2, 1, 3 };

// Instead of
// var sorted = data.OrderBy(e => e);

var sorted = data.Order();

// Instead of
// var sortedDescending = data.OrderByDescending(e => e);

var sortedDescending = data.OrderDescending();
@terrajobst terrajobst added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 26, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Linq untriaged New issue has not been triaged by the area owner labels Mar 26, 2022
@ghost
Copy link

ghost commented Mar 26, 2022

Tagging subscribers to this area: @dotnet/area-system-linq
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Linq provides OrderBy and OrderByDescending which implies that I'm sorting based on some part of the T. Very often I find myself wanting to just sort by the T itself.

API Proposal

namespace System.Linq
{
    public partial class Enumerable
    {
        public static IEnumerable<T> Ordered<T>(this IEnumerable<T> source) where T: IComparable<T>;
        public static IEnumerable<T> OrderedDescending<T>(this IEnumerable<T> source) where T: IComparable<T>;
    }
}

API Usage

var data = new[] { 2, 1, 3 };

// Instead of
// var sorted = data.OrderBy(e => e);

var sorted = data.Ordered();

// Instead of
// var sortedDescending = data.OrderByDescending(e => e);

var sortedDescending = data.OrderedDescending();

Alternative Designs

No response

Risks

No response

Author: terrajobst
Assignees: -
Labels:

api-suggestion, area-System.Linq, untriaged

Milestone: -

@kevingosse
Copy link
Contributor

Shouldn't it return an IOrderedEnumerable<T> for consistency with OrderBy?

@petroemil
Copy link

Could they be called Order and OrderDescending instead, dropping the past tense "ed" from the names?

@stephentoub
Copy link
Member

stephentoub commented Mar 26, 2022

If we add this, I think the name should just be Order rather than Ordered.

@poke
Copy link
Contributor

poke commented Mar 26, 2022

Maybe something explicit to avoid confusion with the existing methods: OrderBySelf and OrderBySelfDescending (OrderByDescendingSelf?)

Alternatively just OrderBy() without any argument, defaulting to an identity function? 🤔

@CamiloTerevinto
Copy link

I think it would make more sense to allow for any T too, just like the existing APIs:

    public partial class Enumerable
    {
        // source.OrderBy(x => x)
        public static IOrderedEnumerable<T> Order<T>(this IEnumerable<T> source) where T: IComparable<T>;

        // source.OrderBy(x => x, comparer)
        public static IOrderedEnumerable<T> Order<T>(this IEnumerable<T> source, IComparer<T> comparer);

        // source.OrderByDescending(x => x, comparer)
        public static IOrderedEnumerable<T> OrderDescending<T>(this IEnumerable<T> source) where T: IComparable<T>;

        // source.OrderByDescending(x => x, comparer)
        public static IOrderedEnumerable<T> OrderDescending<T>(this IEnumerable<T> source, IComparer<T> comparer)
    }

@zoran-horvat
Copy link

Could they be called Order and OrderDescending instead, dropping the past tense "ed" from the names?

I agree, with a note that "Ordered" implies that the method will return data in some form, but does not communicate that there will be a substantial cost disproportional to size of the input. "Order" is suggesting that there is work behind constructing the result.

@ldardick
Copy link

Could they be called Order and OrderDescending instead, dropping the past tense "ed" from the names?

I agree, with a note that "Ordered" implies that the method will return data in some form, but does not communicate that there will be a substantial cost disproportional to size of the input. "Order" is suggesting that there is work behind constructing the result.

Adding up, I feel explicitly defining the critetia (OrderAscending and OrderDescending) feels more natural.

@andi0b
Copy link

andi0b commented Mar 26, 2022

So it more or less comes down to instead of calling a LINQ method with an identity function (x=>x) as parameter, just calling it without a parameter.

There are also other LINQ methods that could use this API change. For example SelectMany(x=>x).

But also ToDictionary(x=>x.key, x=>x.value) for a enumerable of tuples or KeyValuePairs (see #45679)

@terrajobst
Copy link
Member Author

Updated proposal based on your feedback.

@terrajobst terrajobst changed the title [API Proposal]: Enumerable.Ordered and OrderedDescending [API Proposal]: Enumerable.Order and OrderDescending Mar 26, 2022
@Joe4evr
Copy link
Contributor

Joe4evr commented Mar 26, 2022

There are also other LINQ methods that could use this API change. For example SelectMany(x=>x).

That almost happened (settled on the name Flatten(), but it didn't make the cut for .NET 6: #54220).

@Arclight3
Copy link

Arclight3 commented Mar 26, 2022

If we add this, I think the name should just be Order rather than Ordered.

Funny how you posted this and got 8 likes, but someone who said the exact same thing before you got no reactions :))
Btw, why did you said something that he already said?

@terrajobst
Copy link
Member Author

Can’t speak for @stephentoub but it usually happens to me due to overlap (ie while I’m typing someone posts before me).

@stephentoub
Copy link
Member

Btw, why did you said something that he already said?

We posted at the same time.

@Arclight3
Copy link

I see. Please accept my apologies.
The fact that you and Immo explained this to me shows again how cool you guys are, and what a jerk I can be sometimes 😄

@ClosetBugSlayer
Copy link

ClosetBugSlayer commented Mar 27, 2022

Alternatively just OrderBy() without any argument, defaulting to an identity function?

This is what I added to my own libraries years ago and it works fabulously. If it isn't an overload, it won't be discovered, and the programmer won't automatically assume it does the same general thing when reading the code. I am that programmer, speaking from experience.

Side note, I also created a SortBy(...) which operates in-place on lists with a single selector.

@terrajobst
Copy link
Member Author

terrajobst commented Mar 27, 2022

If it isn't an overload, it won't be discovered, and the programmer won't automatically assume it does the same general thing when reading the code.

In principle I agree. However, naming also matters. OrderBy() with no arguments feels quite strange IMHO.

@jzabroski
Copy link
Contributor

.Sort() uses Comparer<T>.Default, so there is some inconsistency here between the mechanism method Sort and the policy method Order

@EluciusFTW
Copy link

EluciusFTW commented Mar 27, 2022

I find Order does not really express what it does, it does not really flow through the chain. I'd actually prefer InAscendingOrder and InDescendingOrder. I think that is most readable, and closest to natural language, in the same way as, e.g., AsEnumerable.
Just compare the statements:

var topThree = sequenceOfNumbers
    .Order()
    .Take(3);

var topThree = sequenceOfNumbers
    .InDescendingOrder()
    .Take(3);

@stephentoub
Copy link
Member

stephentoub commented Mar 27, 2022

From my perspective, such a helper (to avoid x => x) barely rises to the level of something we should be adding. But if it's common enough that we decide to go ahead with it, it's definitely not worth inventing new patterns and naming. We already have the Operation and OperationBy naming pattern, with Max and MaxBy, Distinct and DistinctBy, Intersect and IntersectBy, etc. Since we already have OrderBy that satisfies the latter half of the pattern, this should just be Order (and OrderDescending) to stay with the same pattern, as Immo's proposal now has. Introducing a new name/concept would be confusing with the existing methods (suggesting it somehow has different semantics) and inconsistent with the rest of LINQ

@EluciusFTW
Copy link

@stephentoub I agree that this helper is not really necessary (and anyone can write it on his own, if he uses it a lot). I also understand the reasoning for the naming proposal, just wanted to throw another option out there. I don't agree that it goes against the semanitcs of LINQ, if you compare it to AsEnumerable, AsQueryable. By your argument, these should just have been called Enumerable and Queryable.

@rjgotten
Copy link

I don't agree that it goes against the semanitcs of LINQ, if you compare it to AsEnumerable, AsQueryable. By your argument, these should just have been called Enumerable and Queryable.

I always thought the reason those are As[..] are to juxtapose with To[..] as in ToArray et al.
The 'to' implies actual work in converting the data; the 'as' is more of a reinterpretation, wrapping or cast of the existing data.

@andi0b
Copy link

andi0b commented Mar 28, 2022

First() or SingleOrDefault() and similar functions have the parameterless overload.

Calling it differently than the existing OrderBy() would be even more confusing I would say.

@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Mar 28, 2022
@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Mar 28, 2022
@eiriktsarpalis eiriktsarpalis added blocking Marks issues that we want to fast track in order to unblock other important work and removed blocking Marks issues that we want to fast track in order to unblock other important work labels Jun 7, 2022
@bartonjs
Copy link
Member

bartonjs commented Jun 9, 2022

  • Since this keeps coming up, we recommend first doing an analysis of Enumerable to see if we can do something that is consistent and not explosive, like making selector functions nullable (and defaulted to null).
    • We did the analysis on the call. Losing generic inference makes that a worse approach.
  • We removed the generic constraint both for consistency, and IEnumerable<int?>
  • Should also apply to Queryable
namespace System.Linq
{
    public partial class Enumerable
    {
        public static IOrderedEnumerable<T> Order<T>(this IEnumerable<T> source);
        public static IOrderedEnumerable<T> Order<T>(this IEnumerable<T> source, IComparer<T> comparer);

        public static IOrderedEnumerable<T> OrderDescending<T>(this IEnumerable<T> source);
        public static IOrderedEnumerable<T> OrderDescending<T>(this IEnumerable<T> source, IComparer<T> comparer);
    }
    public partial class Queryable
    {
        // Or whatever's correct.
        [DynamicDependency("Order`1", typeof(Enumerable))]
        public static IOrderedQueryable<T> Order<T>(this IQueryable<T> source);
        [DynamicDependency("Order`1", typeof(Enumerable))]
        public static IOrderedQueryable<T> Order<T>(this IQueryable<T> source, IComparer<T> comparer);

        [DynamicDependency("OrderDescending`1", typeof(Enumerable))]
        public static IOrderedQueryable<T> OrderDescending<T>(this IQueryable<T> source);
        [DynamicDependency("OrderDescending`1", typeof(Enumerable))]
        public static IOrderedQueryable<T> OrderDescending<T>(this IQueryable<T> source, IComparer<T> comparer);
    }
}

@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 Jun 9, 2022
@eiriktsarpalis
Copy link
Member

Based on the decision made today, would it make sense to revisit Flatten() as well?

#54220 (comment)

@stephentoub
Copy link
Member

...and there's the slippery slope. 5 minutes... that took even less time than I expected ;-)

@deeprobin
Copy link
Contributor

Doesn't seem like a big change.
If you are ready for a community contribution here, feel free to assign me.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 9, 2022
@deeprobin
Copy link
Contributor

deeprobin commented Jun 10, 2022

@bartonjs

I don't think there is no way queryable can implement this new API in a more performant way than enumerable now does, as there is no documented way to introspect the comparer and somehow handle that on a repository layer.

In that case: is it useful to add the helper on Queryable?

Originally posted by @rhuijben in #70525 (comment)

@svick
Copy link
Contributor

svick commented Jun 12, 2022

@deeprobin The problem is, if the method is not added to Queryable, what happens if you call queryable.Order()? That code would be valid and would call Enumerable.Order(), since IQueryable<T> inherits from IEnumerable<T>. But then the ordering would not become part of the query, but would be instead executed in memory, which is not what you want.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 30, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Linq
Projects
None yet
Development

Successfully merging a pull request may close this issue.