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

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

Resolve #822

ghost · 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

            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

            /// <param name="equalityComparer">
            /// 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


            /// <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

ghost added area-System.Collections new-api-needs-documentation community-contribution Indicates that the PR has been added by a community member labels Mar 13, 2022

deeprobin suggested changes Mar 14, 2022

View reviewed changes

resolve comments

ec03b19