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

Implement analysis and fix for OrderBy(x => x) to Order() #1522

Merged

Conversation

BenjaminBrienen
Copy link
Contributor

@BenjaminBrienen BenjaminBrienen commented Sep 14, 2024

Fixes #1492

TODO:

  • Unit test fails because the unit test is run with a version of .NET Core where System.Linq does not yet implement IEnumerable<T>.Order().

@BenjaminBrienen
Copy link
Contributor Author

BenjaminBrienen commented Sep 14, 2024

@josefpihrt I added a conditional compilation on the unit test and a feature detection on the analyzer. If you want to handle this a different way, just let me know! :)

Copy link
Collaborator

@josefpihrt josefpihrt left a comment

Choose a reason for hiding this comment

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

Please update changelog.

@BenjaminBrienen
Copy link
Contributor Author

At the moment, I only see methods for adding arguments and ArgumentsList doesn't have a public constructor.

@josefpihrt
Copy link
Collaborator

You can use combination of With... methods and SyntaxFactory methods:

newInvocationExpression = newInvocationExpression.WithArgumentList(
    newInvocationExpression.ArgumentList.WithArguments(
        SyntaxFactory.SeparatedList<ArgumentSyntax>()));

@BenjaminBrienen
Copy link
Contributor Author

You can use combination of With... methods and SyntaxFactory methods:

newInvocationExpression = newInvocationExpression.WithArgumentList(
    newInvocationExpression.ArgumentList.WithArguments(
        SyntaxFactory.SeparatedList<ArgumentSyntax>()));

Ah, I just found another way using RemoveNodes, but this works too! (and it is better)

@BenjaminBrienen
Copy link
Contributor Author

I've doubled checked that all tests pass, so it should be ready for final review.

@josefpihrt
Copy link
Collaborator

Build is failing.

@BenjaminBrienen
Copy link
Contributor Author

Build is failing.

Oh wow, it fails even with a little info message. Good to know there are tight checks! Fixed.

@josefpihrt josefpihrt merged commit 9335c71 into dotnet:main Sep 15, 2024
17 checks passed
@josefpihrt
Copy link
Collaborator

Great PR! Thanks for the contribution! 👍

@BenjaminBrienen BenjaminBrienen deleted the features/simplify-orderby-identity branch September 15, 2024 11:50
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.

RCS1077 for OrderBy(x => x)
2 participants