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 Implementation]: Add Order and OrderDescending to Enumerable and Queryable #70525

Merged
merged 19 commits into from
Jun 30, 2022

Conversation

deeprobin
Copy link
Contributor

@deeprobin deeprobin commented Jun 9, 2022

Proposal implementation of #67194 (closes #67194)

Proposal

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
    {
        [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);
    }
}

Current state of implementation

  • Proposal implementation
  • Ref assembly
  • Tests 1
  • Documentation 2
  • .NET announcement

/cc @terrajobst

Footnotes

  1. I have implemented only the most important test cases, as I would imagine that the complex cases are flaky.
    If desired, I can also implement more test cases.

  2. Copied from the Microsoft Docs from the OrderBy overloads and adjusted accordingly.

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 9, 2022
@ghost
Copy link

ghost commented Jun 9, 2022

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

Issue Details

Proposal implementation of #67194 closes #67194)

Proposal

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);
    }
}

Current state of implementation

  • Proposal implementation 1
  • Ref assembly
  • Tests 2
  • Documentation 3
Author: deeprobin
Assignees: -
Labels:

area-System.Linq, new-api-needs-documentation

Milestone: -

Footnotes

  1. One possibility would of course be to simply forward Order to OrderBy(static item => item), but I guess this won't be really performant, so I implemented it this way (using OrderedKeylessEnumerable).
    I would like to get some feedback.

  2. I have implemented only the most important test cases, as I would imagine that the complex cases are flaky.
    If desired, I can also implement more test cases.

  3. Copied from the Microsoft Docs from the OrderBy overloads and adjusted accordingly.

@rhuijben
Copy link

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?

@deeprobin deeprobin requested a review from eiriktsarpalis June 13, 2022 20:39
@eiriktsarpalis eiriktsarpalis added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 21, 2022
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 21, 2022
@deeprobin deeprobin requested a review from eiriktsarpalis June 28, 2022 20:18
@deeprobin deeprobin requested a review from eiriktsarpalis June 29, 2022 20:01
@deeprobin
Copy link
Contributor Author

; expected
@eiriktsarpalis If you like you can fix the semicolons. I would get to it this evening (CET) at the earliest.

@eiriktsarpalis
Copy link
Member

; expected

Ugh. I copied and pasted the code from main but it looks like fat arrow formatting was different. Should be fixed now.

@deeprobin
Copy link
Contributor Author

@eiriktsarpalis Thank you. I think this is ready-to-merge. Or do you have concerns (then we could ask a 2nd reviewer)?

@deeprobin
Copy link
Contributor Author

I think this API might not be bad for the announcements in the next preview version. Many people work with LINQ so this is probably a relatively interesting API.

@eiriktsarpalis
Copy link
Member

@deeprobin Would you like to add a short announcement in the Preview 7 issue?

dotnet/core#7455

@deeprobin
Copy link
Contributor Author

@eiriktsarpalis dotnet/core#7455 (comment) suggestions?

@eiriktsarpalis
Copy link
Member

suggestions?

Looks good overall, I would skip the API shape and just focus on the final code sample. It should be enough to communicate the background and motivation.

@deeprobin
Copy link
Contributor Author

suggestions?

Looks good overall, I would skip the API shape and just focus on the final code sample. It should be enough to communicate the background and motivation.

I removed the api shape 👍🏼

@eiriktsarpalis
Copy link
Member

Thanks @deeprobin !

@eiriktsarpalis eiriktsarpalis merged commit 436ee2e into dotnet:main 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
area-System.Linq community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Enumerable.Order and OrderDescending
4 participants