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

Add ToLookupAsync extension methods for IQueryable #13623

Merged
merged 1 commit into from
Oct 24, 2018

Conversation

huysentruitw
Copy link
Contributor

@huysentruitw huysentruitw commented Oct 14, 2018

Needed this in a lot of cases already. The latest need was to fetch data for a GraphQL CollectionBatchDataLoader.

Copy link
Contributor

@divega divega left a comment

Choose a reason for hiding this comment

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

PR looks good in general. It is missing test coverage but apparently that is true in general for many of the existing extension methods. @maumar will do some smoke testing and create an issue to add more automated coverage on our stack of these operators.

@huysentruitw
Copy link
Contributor Author

huysentruitw commented Oct 16, 2018

I've searched for ToDictionaryAsync tests, but only found those SourceNonAsyncEnumerableTest's where I've added ToLookupAsync. You can tag me in that issue for adding tests of these extension methods.

@ajcvickers ajcvickers changed the base branch from release/2.2 to master October 23, 2018 22:31
@ajcvickers
Copy link
Contributor

Changing base to master to merge for 3.0.

@divega
Copy link
Contributor

divega commented Oct 23, 2018

@maumar can you please write down what you found with the investigation you did about this? Do we need new issues to track missing test coverage.

@divega
Copy link
Contributor

divega commented Oct 23, 2018

@huysentruitw Thanks a lot for submitting this PR. At this point we are locking down 2.2, so we will consider it for 3.0.

@pbarranis
Copy link

It seems like these extension methods were removed from EntityFrameworkQueryableExtensions.cs sometime between when they were merged to master and when the file was moved from src/EFCore/ to src/EFCore/Extensions. Was this intentional?

I'm afraid I don't know how to get at the history of EntityFrameworkQueryableExtensions.cs on GitHub before it was moved. From what I'm reading online it's not possible; you have to have the repo locally and use git commands to investigate further.

@smitpatel
Copy link
Contributor

@pbarranis - We removed it intentionally. It was removed in 3.0 release. And we decided not to add it again as per #15916. Removed in #15983

In order to use Take EF Core queryable and enumerate it using AsEnumerable/AsAsyncEnumerable. Then you can use ToLookup API from other libraries (sync from corefx, async from IX-Async).

@huysentruitw
Copy link
Contributor Author

I took the same approach as ToDictionary. Why remove one but not the other? You can as well remove ToList, ToArray and ToDictionary as well then, right?

@smitpatel
Copy link
Contributor

@huysentruitw - Your approach was same when EF Core used IX-Async underneath. EF Core 3.x onwards it does not. For ToList/ToArray/ToDictionary, it is easy to implement it from corefx library. ToLookup requires access to Lookup which is defined internal in corefx. As noted explicitly in the issue I linked, we decided not to define that ourselves. If Dictionary<,> was internal too, we would remove that too.

@huysentruitw
Copy link
Contributor Author

Makes sense, thanks for the explanation 👍

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.

5 participants