Align API surface of immutable collections and their corresponding builder types #66550

RaymondHuy · 2022-03-13T04:59:51Z

Resolve #822

dotnet-issue-labeler · 2022-03-13T04:59:57Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2022-03-13T05:00:02Z

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

Issue Details

Resolve #822

Author:	RaymondHuy
Assignees:	-
Labels:	`area-System.Collections`, `new-api-needs-documentation`, `community-contribution`
Milestone:	-

...es/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.Builder.cs

src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.cs

...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs

RaymondHuy · 2022-03-25T09:38:32Z

Hi @eiriktsarpalis Could you take a look on this PR ? 😃

eiriktsarpalis · 2022-03-25T11:24:00Z

@RaymondHuy I'm backlogged at the moment, will follow up with reviewing later. Thanks.

src/libraries/System.Collections.Immutable/ref/System.Collections.Immutable.cs

...es/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.Builder.cs

...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs

...es/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.Builder.cs

src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.cs

...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs

...ystem.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Builder.cs

src/libraries/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs

eiriktsarpalis

Some minor feedback concerning implementation and XML documentation. CI also seems to be failing because of parameter mismatches between reference source and implementation.

src/libraries/System.Collections.Immutable/ref/System.Collections.Immutable.cs

eiriktsarpalis · 2022-04-21T10:32:31Z

src/libraries/System.Collections.Immutable/ref/System.Collections.Immutable.cs

@@ -702,8 +720,14 @@ public sealed partial class Builder : System.Collections.Generic.ICollection<T>,
            public int LastIndexOf(T item, int startIndex, int count) { throw null; }
            public int LastIndexOf(T item, int startIndex, int count, System.Collections.Generic.IEqualityComparer<T>? equalityComparer) { throw null; }
            public bool Remove(T item) { throw null; }
+            public void Remove(T value, IEqualityComparer<T>? equalityComparer) { throw null; }


Similar to the above, cc @terrajobst

Suggested change

public void Remove(T value, IEqualityComparer<T>? equalityComparer) { throw null; }

public bool Remove(T item, IEqualityComparer<T>? equalityComparer) { throw null; }

@RaymondHuy could you please update the return type? It should match that of the existing overload.

Thanks @eiriktsarpalis I have updated the PR.

src/libraries/System.Collections.Immutable/ref/System.Collections.Immutable.cs

eiriktsarpalis · 2022-05-04T08:18:26Z

...es/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.Builder.cs

@@ -486,14 +487,17 @@ public bool Remove(T element)
            /// The equality comparer to use in the search.
            /// If <c>null</c>, <see cref="EqualityComparer{T}.Default"/> is used.
            /// </param>


Please add a <returns /> element as above.

eiriktsarpalis · 2022-05-04T08:18:42Z

...es/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.Builder.cs

@@ -463,6 +463,7 @@ public void AddRange(Builder items)

            /// <summary>
            /// Removes the first occurrence of the specified element from the builder.
+            /// If no match is found, the builder remains unchanged.


If we're adding this, shouldn't it be included in the new overload as well?

src/libraries/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs

eiriktsarpalis

Thanks!

RaymondHuy added 5 commits February 6, 2022 13:01

Add more ImmutableArray api

9b8417f

add immutable list builder api

22e3f6d

Add Immutable Array Builder api

5f0776e

Merge branch 'master' into issue-822

a07edda

remove unused namespace

97e2488

dotnet-issue-labeler bot added area-System.Collections new-api-needs-documentation labels Mar 13, 2022

ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 13, 2022

deeprobin suggested changes Mar 14, 2022

View reviewed changes

resolve comments

ec03b19