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

Immutable Collections - anyone else *need* this? #42

Open
shiftkey opened this issue Nov 16, 2013 · 3 comments
Open

Immutable Collections - anyone else *need* this? #42

shiftkey opened this issue Nov 16, 2013 · 3 comments

Comments

@shiftkey
Copy link
Contributor

Octokit.NET uses SimpleJson heavily, and with the license changes to Microsoft.Bcl.Immutable we can now use the System.Collections.Immutable namespaces rather than our custom implementations

BUT

There's a whole bunch of changes to how you can create ImmutableColllections (that is, the concrete implementations of IImmutableCollection) that break existing assumptions.

public interface IImmutableDictionary<TKey, TValue> 
    : IReadOnlyDictionary<TKey, TValue>,
      IReadOnlyCollection<KeyValuePair<TKey, TValue>>,
      IEnumerable<KeyValuePair<TKey, TValue>>, 
      IEnumerable

versus

public interface IDictionary<TKey, TValue> 
    : ICollection<KeyValuePair<TKey, TValue>>, 
      IEnumerable<KeyValuePair<TKey, TValue>>,
      IEnumerable

for example.

I've made some progress here on getting this scenario running, but it's not ready for a review or even a PR.

Thoughts?

@prabirshrestha
Copy link
Member

I would definitely be interested in getting this in the core.

  • Since IImmutableDictionary requires new references, we would need an option to disable/enable it. (Default should be disabled). Would need something like #define SIMPLE_JSON_IMMUTABLE_COLLECTIONS.
  • Also we can't use vars :( as we need to support the ISO-2 C# lang spec.
  • Any reflection utils changes should go in this reflection-utils first before I merge it here. (But that is done once we are sure we are gonna use it in simple-json. So you can send PR in reflection-utils once we decide to merge it here)
  • also would be good to squeeze in ImmutableList<T> for arrays.

I will be gone for MVP summit this week, so not sure if I will be able to look at your PRs or dive into this more, but please feel free to submit a PR.

@shiftkey
Copy link
Contributor Author

@prabirshrestha i'll also be around for MVP summit. It's not urgent, but I'll keep that feedback in mind...

@prabirshrestha
Copy link
Member

@shiftkey cool. we should meet. feel free to ping me on twitter. same username as github.

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

No branches or pull requests

2 participants