Implement LINQ LeftJoin and RightJoin#110872
Conversation
|
Note regarding the |
1 similar comment
|
Note regarding the |
There was a problem hiding this comment.
Copilot reviewed 7 out of 14 changed files in this pull request and generated 2 comments.
Files not reviewed (7)
- src/libraries/System.Linq.Queryable/tests/System.Linq.Queryable.Tests.csproj: Language not supported
- src/libraries/System.Linq/src/System.Linq.csproj: Language not supported
- src/libraries/System.Linq/tests/System.Linq.Tests.csproj: Language not supported
- src/libraries/System.Linq/ref/System.Linq.cs: Evaluated as low risk
- src/libraries/System.Linq.Queryable/ref/System.Linq.Queryable.cs: Evaluated as low risk
- src/libraries/System.Linq/tests/JoinTests.cs: Evaluated as low risk
- src/libraries/System.Linq.Queryable/tests/JoinTests.cs: Evaluated as low risk
Comments suppressed due to low confidence (3)
src/libraries/System.Linq/src/System/Linq/RightJoin.cs:43
- The IsEmptyArray method is not defined in the provided code. Ensure that this method is defined and works as expected.
if (IsEmptyArray(inner))
src/libraries/System.Linq.Queryable/src/System/Linq/Queryable.cs:245
- The DynamicDependency attribute should be corrected to use the format: DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(Enumerable)).
[DynamicDependency("LeftJoin`4", typeof(Enumerable))]
src/libraries/System.Linq.Queryable/src/System/Linq/Queryable.cs:260
- The nullable type for q is not necessary. Use a simple cast instead: IQueryable q = source as IQueryable;
IQueryable<TSource>? q = source as IQueryable<TSource>;
|
Hey @eiriktsarpalis, thanks for the quick reviewing. Re the styling nits, I basically tried to follow the existing code (Join implementation and tests) for maximum consistency, although there are indeed many opportunities for syntax modernization (namespace declaration, using declaration, the new collection initializers...). I'm happy to make the code more modern for Left/RightJoin in this PR only as you suggest (at which point things would be inconsistent), or I can submit a separate PR later to do some syntax cleanup for System.Linq as a whole - whatever you prefer. |
That's fine, they're all optional. Assuming we do add enforcement in the future I'm guessing they would be updated in bulk anyway. |
| { | ||
| public static partial class Enumerable | ||
| { | ||
| public static IEnumerable<TResult> LeftJoin<TOuter, TInner, TKey, TResult>(this IEnumerable<TOuter> outer, IEnumerable<TInner> inner, Func<TOuter, TKey> outerKeySelector, Func<TInner, TKey> innerKeySelector, Func<TOuter, TInner?, TResult> resultSelector) => |
There was a problem hiding this comment.
We should add XML doc comments to all of the new public APIs.
There was a problem hiding this comment.
Ah, I was following the existing code on this (e.g. Join has no xmldocs) - are we already in a place where the API docs are generated from the xmldocs?
In any case, I'll take care of this.
There was a problem hiding this comment.
are we already in a place where the API docs are generated from the xmldocs?
Yes and no.
Some libraries have the src as the source of truth for docs. Unfortunately, System.Linq isn't yet one of them. However, even for such libraries, we add XML comments for all new surface area, and those comments then seed the docs for those methods, even if they're not then the source of truth moving forward.
Closes #110292