Initial open-sourcing commit of the MultiValueDictionary #370
Conversation
The MultiValueDictionary is an experimental collection that was first released the summer of 2014 as a Nuget package. This commit releases the source code of the MultiValueDictionary tied into .NET Core with the intention of it being considered as an addition to System.Collections.Generic in CoreFX. - All code is open sourced from the original package with only changes made to get it functioning. As such, some things will need to be changed to better match the style of CoreFX (e.g. the tests). - I based the csproj, project.json, folder structure, and strings.resx heavily on that of System.Collections.Generic in CoreFX so that they may be easily merged in the future. This is why Strings.resx has some strings that aren't used by the MVD - Addition of performance tests required an update of BuildTools from 70 to 107. - The MVD released earlier had tie-in functions to ILookup like ConvertToLookup. I removed those with the intention of adding them as extension functions in ILookup instead of being in MultiValueDictionary. The reason for this is that System.Collections.Generic shouldn't reference System.Linq but System.Linq can reference System.Collections.Generic. - The design decisions around the MVD were made at point where .NET was very different than it is today. Some things might be worth reconsidering now that .NET Core exists.
| /// The function to construct a new <see cref="ICollection{TValue}"/> | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| private Func<ICollection<TValue>> NewCollectionFactory = () => new List<TValue>(); |
There was a problem hiding this comment.
Should this be static?
There was a problem hiding this comment.
The purpose of the NewCollectionFactory is to allow an instance of a MultiValueDictionary to be created that has a custom specified inner collection. I set it here to new List() as the default, but I don't think it should be static. It would make sense that you may want a MultiValueDictionary of Lists as well as a MultiValueDictionary of HashSets in the same program. The static Factories starting on line 357 are an example of when this would be used.
As an aside, if I remember correctly we also discussed removing the public NewCollectionFactory constructors in favor of a bool "AllowRepeatedValues" that would act as a simple function to choose between HashSet and List as the internal structure. It was decided on giving the user more control through the current pattern though.
There was a problem hiding this comment.
OK. Thanks for the explanation. I think it's fine for this initial checkin. But we should discuss at the API review if we think this feature (ability to change the type of the collection) is really needed /used. This feature has a non-trivial cost per instance of the dictionary and I am almost sure most of the instances use the default collection type.
There was a problem hiding this comment.
I agree. It's a pretty specific feature for such a general collection and I it may just add unnecessary confusion to what should be a simple decision of "do I want to allow repeated TValues for a given TKey".
|
BTW, please feel free to merge in, just open an issue to discuss the two design points at the API review. |
|
Thanks @KrzysztofCwalina will do |
Initial open-sourcing commit of the MultiValueDictionary
The MultiValueDictionary is an experimental collection that was first released the summer of 2014 as a Nuget package. This commit releases the source code of the MultiValueDictionary tied into .NET Core with the intention of it being considered as an addition to System.Collections.Generic in CoreFX.