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 ToDictionaryAsync and ToHashSetAsync Linq extension methods #3503

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dennis-gr
Copy link
Contributor

Convenience methods to avoid the slightly awkward syntax when doing it manually after calling ToListAsync.

Async-Generator fails to generate tests for ToDictionaryAsync, not sure why it doesn't get recognized.

@fredericDelaporte
Copy link
Member

fredericDelaporte commented Apr 28, 2024

What is awkward? (await query.ToListAsync()).ToHashSet()? Or hashSet.UnionWith(await query.ToListAsync())?

I do not see the need.

@dennis-gr
Copy link
Contributor Author

What is the awkward? (await query.ToListAsync()).ToHashSet()? Or hashSet.UnionWith(await query.ToListAsync())?

Both: the chained method call with the need to add parantheses around the async call, creating the HashSet yourself also isn't great IMHO.

It's a small usability improvement, I don't see the harm in adding these.

@fredericDelaporte
Copy link
Member

I see some harm:

  • It may lure users into thinking the thing is optimized in order to directly load the result into a hashset/dictionary instead of going through an intermediate list.
  • That is still some additional code to maintain, just for sparing a couple of parenthesis to callers.

Other maintainers may see it otherwise, so, It leave it open.

@dennis-gr
Copy link
Contributor Author

I see some harm:

* It may lure users into thinking the thing is optimized in order to directly load the result into a hashset/dictionary instead of going through an intermediate list.

Optimizing this would be possible as soon as #2274 is done. I'll add a remark that calling these methods still creates an intermediate list.

* That is still some additional code to maintain, just for sparing a couple of parenthesis to callers.

Like I said, ATM it's a small usability improvement that I think is worth it, with the option to provide a slightly optimized version in the future. EF Core has these methods and it would be nice if NH did too.

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.

2 participants