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 Proposal]: Add overload of ToDictionary that turns an IEnumerable of KeyValuePair or ValueTuple #65925

Closed
davidfowl opened this issue Feb 27, 2022 · 19 comments · Fixed by #85811
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@davidfowl
Copy link
Member

davidfowl commented Feb 27, 2022

Background and motivation

Sometimes in parsing it's common transform key value pairs expressed as text into a dictionary pretty concise LINQ statement: Consider a query string parser (a=b&c=d&e=f).

Dictionary<string, string> Parse(string query) =>
    query.Split('&')
    .Select<string, (string k, string v)?>(s => s.Split('=') switch // This part is gross 😢 
    {
        [var k, var v] => (k, v),
        _ => null,
    })
    .Where(p => p is not null)
    .Select(p => p!.Value)
    .ToDictionary(p => p.k!, p => p.v!);

The last ToDictionary call wouldn't require explicitly mapping the key and value for this common case.

API Proposal

namespace System.Collections.Generic;

public static partial class Enumerable
{
    public static Dictionary<TKey, TValue> ToDictionary<TKey, TValue>(this IEnumerable<KeyValuePair<TKey, TValue>> source) where TKey : notnull;
    public static Dictionary<TKey, TValue> ToDictionary<TKey, TValue>(this IEnumerable<ValueTuple<TKey, TValue>> source) 
where TKey : notnull;
    public static Dictionary<TKey, TValue> ToDictionary<TKey, TValue>(this IEnumerable<ValueTuple<TKey, TValue>> source, IEqualityComparer<TKey> comparer) where TKey : notnull;
}

API Usage

// Fancy the value
var c = new List<(string, int)>();
c.Add(("one", 1));
c.Add(("two", 2));
c.Add(("three", 3));

var map = c.ToDictionary();

// Getting the values out
foreach (var (key, value) in map)
    Console.WriteLine($"{key} = {value}");

Alternative Designs

No response

Risks

No response

@davidfowl davidfowl added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Feb 27, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Collections untriaged New issue has not been triaged by the area owner labels Feb 27, 2022
@ghost
Copy link

ghost commented Feb 27, 2022

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

Issue Details

Background and motivation

Sometimes in parsing it's common transform key value pairs expressed as text into a dictionary pretty concise LINQ statement: Consider a query string parser (a=b&c=d&e=f).

Dictionary<string, string> Parse(string query) =>
    query.Split('&')
    .Select<string, (string? k, string? v)?>(s => s.Split('=') switch // This part is gross 😢 
    {
        [var k, var v] => (k, v),
        _ => null,
    })
    .Where(p => p is not null)
    .Select(p => p!.Value)
    .ToDictionary(p => p.k!, p => p.v!);

The last ToDictionary call wouldn't require explicitly mapping the key and value for this common case.

API Proposal

namespace System.Collections.Generic;

public static partial class Enumerable
{
    public static Dictionary<TKey, TSource> ToDictionary<TSource, TKey>(this IEnumerable<KeyValuePair<TSource, TKey>> source) where TKey : notnull;
    public static Dictionary<TKey, TSource> ToDictionary<TSource, TKey>(this IEnumerable<ValueTuple<TSource, TKey>> source) where TKey : notnull;
}

API Usage

// Fancy the value
var c = new List<(string, int)>();
c.Add(("one", 1));
c.Add(("two", 2));
c.Add(("three", 3));

var map = c.ToDictionary();

// Getting the values out
foreach (var (key, value) in map)
    Console.WriteLine($"{key} = {value}");

Alternative Designs

No response

Risks

No response

Author: davidfowl
Assignees: -
Labels:

api-suggestion, area-System.Collections, untriaged

Milestone: -

@DrkWzrd
Copy link

DrkWzrd commented Feb 27, 2022

I think TKey is misplaced and TSource is misleading. What should last code print in your opinion? "one = 1"? or "1 = one"?

In linq naming conventions, TSource would be KVP<K, V> or VT<V1, V2>, not K or V1.

@davidfowl
Copy link
Member Author

davidfowl commented Feb 27, 2022

What should last code print in your opinion? "one = 1"? or "1 = one"?

The most intuitive thing would be one = 1 😄. At least that was the intent.

What should last code print in your opinion? "one = 1"? or "1 = one"?

Sure, I don't have any hangups about the generic argument names, whatever works. The intent is that the first value of the tuple is the key (Item1) and the second is the value (Item2). I see the issue. Fixed the generic arguments.

@jnyrup
Copy link
Contributor

jnyrup commented Feb 27, 2022

Do you envision other benefits from this besides the fewer key strokes?

Should there also be overloads taking an IEqualityComparer<TKey>?

@eiriktsarpalis
Copy link
Member

Sounds reasonable, proposal should probably also include equivalent IQueryable overloads cc @krwq

@davidfowl
Copy link
Member Author

davidfowl commented Mar 1, 2022

Do you envision other benefits from this besides the fewer key strokes?

Haven't thought about the other potential savings.

Should there also be overloads taking an IEqualityComparer<TKey>?

Possibly yes.

Sounds reasonable, proposal should probably also include equivalent IQueryable overloads cc @krwq

Do we also have ToDictionary on IQueryable? Seems strange no?

@jnyrup
Copy link
Contributor

jnyrup commented Mar 1, 2022

I'm asking since I recall(?) the bar of adding generic overloads to Enumerable has been a bit high and this doesn't seem to add new functionality.

@eiriktsarpalis
Copy link
Member

Do we also have ToDictionary on IQueryable? Seems strange no?

Ah yes you're right, not applicable to To[Collection] methods.

@krwq krwq added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Mar 4, 2022
@krwq krwq added this to the Future milestone Mar 4, 2022
@krwq
Copy link
Member

krwq commented Mar 4, 2022

The proposal seems complete and I'm not strongly opinionated on this so I'll let API reviewers decide if we want this or not. I've personally wrote similar code in the past as mentioned so I guess it is occasionally very useful.

@stephentoub
Copy link
Member

stephentoub commented Mar 15, 2022

To make sure I understand the proposal, this would simply be to enable you to write:

.ToDictionary()

instead of

.ToDictionary(p => p.Key, p => p.Value)

? Is this scenario really common enough to warrant that? If you're parsing and you care about perf, you shouldn't be using LINQ for that in the first place, so presumably perf isn't really the argument here, and it's just the usability / discoverability? Personally I'd want to see compelling evidence that this is a common use case. (I see some examples in source.dot.net, but a bunch of the uses of that overload actually transform the key or value, while others shouldn't be using ToDictionary at all and should instead just use Dictionary's copy ctor.)

@davidfowl
Copy link
Member Author

Yes I have no idea how common it is in the wild, I've been using this pattern more recently with tuples and LINQ so I figured I'd file an issue.

@eiriktsarpalis
Copy link
Member

The proposal should probably include overloads that accept an IEqualityComparer<TKey> parameter.

@davidfowl
Copy link
Member Author

Updated

@Neme12
Copy link

Neme12 commented May 4, 2022

IMO we should use named tuples wherever possible, so this would be IEnumerable<(TKey key, TValue)> instead of IEnumerable<ValueTuple<TKey, TValue>>

@Neme12
Copy link

Neme12 commented May 4, 2022

It might be worth mentioning that there is already an extension method ImmutableDictionary.ToImmutableDictionary that takes IEnumerable<KeyValuePair<TKey, TValue>>. I think there should be some parity between these extension methods and their overloads.

There's another issue that suggests adding overloads that take ValueTuple in addition to the existing overloads that take KeyValuePair of ToImmutableDictionary: #68811. I think both of these should be looked at at the same time and the outcome should be parity between ToDictionary and ToImmutableDictionary.

@Neme12
Copy link

Neme12 commented May 4, 2022

@davidfowl
You only added that overload for the ValueTuple variant, not for the KeyValuePair one. Also, it should most likely be nullable like in the existing overloads that take a comparer.

@TonyValenti
Copy link

I could definitely use this as I've already written the code to do these things. It makes initializing a dictionary so much more convenient.

@bartonjs
Copy link
Member

bartonjs commented Apr 27, 2023

Video

  • We changed the ValueTuple<TKey, TValue> syntax to (TKey Key, TValue Value)
  • We added the KVP+EqualityComparer overload to square off the proposal.
  • Discussion brought up that dict.ToDictionary() looks like a clone, but actually always uses the default comparer, which might be a loss of data integrity... and this should be pointed out in docs.
namespace System.Collections.Generic;

public static partial class Enumerable
{
    public static Dictionary<TKey, TValue> ToDictionary<TKey, TValue>(this IEnumerable<KeyValuePair<TKey, TValue>> source) where TKey : notnull;
    public static Dictionary<TKey, TValue> ToDictionary<TKey, TValue>(this IEnumerable<KeyValuePair<TKey, TValue>> source, IEqualityComparer<TKey> comparer) where TKey : notnull;
    public static Dictionary<TKey, TValue> ToDictionary<TKey, TValue>(this IEnumerable<(TKey Key, TValue Value)> source) where TKey : notnull;
    public static Dictionary<TKey, TValue> ToDictionary<TKey, TValue>(this IEnumerable<(TKey Key, TValue Value)> source, IEqualityComparer<TKey> comparer) where TKey : notnull;
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Apr 27, 2023
@eiriktsarpalis eiriktsarpalis added the help wanted [up-for-grabs] Good issue for external contributors label Apr 28, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 5, 2023
@lateapexearlyspeed
Copy link
Contributor

Hi, PR is marked as ready for review now, thanks.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 17, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 16, 2023
@davidfowl davidfowl modified the milestones: Future, 8.0.0 Jul 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants