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 extension method AsReadOnly() for IDictionary<TKey, TValue> #33033

Closed
Mrxx99 opened this issue Mar 1, 2020 · 11 comments · Fixed by #61172
Closed

[API Proposal]: Add extension method AsReadOnly() for IDictionary<TKey, TValue> #33033

Mrxx99 opened this issue Mar 1, 2020 · 11 comments · Fixed by #61172
Assignees
Labels
api-approved API was approved in API review, it can be implemented api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@Mrxx99
Copy link
Contributor

Mrxx99 commented Mar 1, 2020

Background and motivation

Like the List<T> has a method AsReadOnly() I would like to propose the same for the Dictionary.
This could lead to more clean and fluent code, when you can call AsReadOnly at the end of a call chain instead of creating a new instance of ReadOnlyDictionary manually.

I am aware that Dictionary already implements IReadOnlyDictionary but only using the ReadOnlyDictionary it is really immutable without the possibility to cast it back to (I)Dictionary.

API Proposal

namespace System.Collections.Generic
{
    public static class CollectionExtensions
    {
+       public static ReadOnlyDictionary<TKey, TValue> AsReadOnly(this IDictionary<TKey, TValue> dictionary)
+            => new ReadOnlyDictionary<TKey, TValue>(dictionary);
    }
}

API Usage

private Dictionary<string, string> _theDictionary = new Dictionary<string, string>();

public IReadOnlyDictionary<string, string> TheDictionary => _theDictionary.AsReadOnly();

Alternative Designs

The origianl proposal (as can be read in the comments, an extension method is preferred):

public class Dictionary<TKey, TValue> : IDictionary<TKey, TValue>, IDictionary...
{
+        public ReadOnlyDictionary<TKey, TValue> AsReadOnly()
+            => new ReadOnlyDictionary<TKey, TValue>(this);
}

Risks

I don't see any it's just one extension method more calling a very common constructor.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Collections untriaged New issue has not been triaged by the area owner labels Mar 1, 2020
@DevDrake
Copy link

DevDrake commented Mar 3, 2020

I was scratching my head thinking about this proposal.
If we need the functionality i would be more opt to extension function or default interface implementation for IDictionary<,>
public ReadOnlyDictionary<TKey, TValue> AsReadOnly(this IDictionary<TKey, TValue> dictionary) => new ReadOnlyDictionary<TKey, TValue>(dictionary)
as this will cover more than just Dictionary.
also this is already used in runtime/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/MetadataServices.cs for example.
I'm not very convinced we need it at all.
IMHO we should go more in the #31573 direction

@layomia layomia added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 24, 2020
@layomia layomia added this to the Future milestone Jun 24, 2020
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Jun 24, 2020
@eiriktsarpalis
Copy link
Member

We'd probably want to express this as an extension method due to codegen size concerns. How about this?

namespace System.Collections.Generic
{
    public static class CollectionExtensions
    {
+       public static ReadOnlyDictionary<TKey, TValue> AsReadOnly(this IDictionary<TKey, TValue> dictionary)
+            => new ReadOnlyDictionary<TKey, TValue>(dictionary);
    }
}

cc @jkotas @GrabYourPitchforks

@Mrxx99
Copy link
Contributor Author

Mrxx99 commented Nov 24, 2020

I agree, I think it does make more sense as extension method.

@mattjohnsonpint
Copy link
Contributor

Is this API still up for consideration?

@eiriktsarpalis
Copy link
Member

It is, but we're not prioritizing it for .NET 6.

@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 7.0.0 Oct 29, 2021
@eiriktsarpalis
Copy link
Member

@Mrxx99 would it be possible to update your API proposal following the API proposal template? Please also change the API diff so that it's expressed as an extension method as #33033 (comment)

@Mrxx99 Mrxx99 changed the title Add Dictionary<T>.AsReadOnly() [API Proposal]: Add Dictionary<T>.AsReadOnly() Oct 29, 2021
@Mrxx99
Copy link
Contributor Author

Mrxx99 commented Oct 29, 2021

@eiriktsarpalis done.

@ghost ghost added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs more info labels Oct 29, 2021
@Mrxx99 Mrxx99 changed the title [API Proposal]: Add Dictionary<T>.AsReadOnly() [API Proposal]: Add extension method AsReadOnly() for IDictionary<TKey, TValue> Oct 30, 2021
@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Nov 1, 2021
@eiriktsarpalis eiriktsarpalis self-assigned this Nov 1, 2021
@terrajobst
Copy link
Contributor

terrajobst commented Nov 2, 2021

Video

  • This makes sense, but it seems we should be consistent and offer one for ReadOnlyCollection<T>, which today only exists as instance methods on List<T> and Array.
namespace System.Collections.Generic
{
    public static class CollectionExtensions
    {
        public static ReadOnlyCollection<T> AsReadOnly<T>(this IList<T> list);
        public static ReadOnlyDictionary<TKey, TValue> AsReadOnly<TKey, TValue>(this IDictionary<TKey, TValue> dictionary);
    }
}

@terrajobst terrajobst 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 Nov 2, 2021
@eiriktsarpalis
Copy link
Member

@Mrxx99 would you be interested in providing an implementation?

@eiriktsarpalis eiriktsarpalis added the help wanted [up-for-grabs] Good issue for external contributors label Nov 2, 2021
@Mrxx99
Copy link
Contributor Author

Mrxx99 commented Nov 2, 2021

@eiriktsarpalis Yes, I would be interested. Also thanks for moving this issue forward. Should I also update the proposal to also include the extension method on IList?

@eiriktsarpalis
Copy link
Member

Should I also update the proposal to also include the extension method on IList?

No that's fine, we use #33033 (comment) as the source of truth.

@eiriktsarpalis eiriktsarpalis removed their assignment Nov 2, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 3, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 23, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2021
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 api-suggestion Early API idea and discussion, it is NOT ready for implementation 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.

7 participants