Skip to content
This repository has been archived by the owner on Sep 13, 2022. It is now read-only.

Add bulk operations to Collection<T> #1175

Merged
merged 1 commit into from
May 9, 2019

Conversation

terrajobst
Copy link
Member

These APIs were recently merged into CoreFx.

This fixes #1091.

@terrajobst terrajobst requested review from a team as code owners May 2, 2019 21:23
@terrajobst terrajobst added the netstandard-api This tracks requests for standardizing APIs. label May 2, 2019
@terrajobst terrajobst added this to the .NET Standard 2.1 milestone May 2, 2019
@SkyeHoefling
Copy link

This looks great! Thanks for getting this done so quickly after we wrapped up the corefx PR

public bool Remove(T item) { throw null; }
public void RemoveAt(int index) { }
protected virtual void RemoveItem(int index) { }
protected virtual void RemoveItemsRange(int index, int count) { }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why this and APIs bellow don't use Range type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this API was approved 3 years ago but only recently implemented. Not sure it's worth going back to changing the design. We can always add new overloads.

@terrajobst
Copy link
Member Author

terrajobst commented May 9, 2019

@joshpeterson @jaredpar ...while you're at it... :-)

@@ -1,3 +1,10 @@
Compat issues with assembly System.ComponentModel.TypeConverter:
CannotSealType : Type 'System.ComponentModel.BaseNumberConverter' is effectively (has a private constructor) sealed in the implementation but not sealed in the contract.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericstj @wtgodbe any idea where these are coming from? Those changes are new after I rebased to master. Did Arcade change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Anipik added better checks to catch the exact case that resulted in this bug.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this baseline file indicate BTW? netstandard 1.6 didn't have BaseNumberConverter.

Copy link
Member

@ericstj ericstj May 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it did: https://github.com/dotnet/corefx/blob/82cce14f0c8188a2218ec5dc9a55977ccdba02e0/src/System.ComponentModel.TypeConverter/ref/project.json#L11

Or rather, we had a package that was applicable on netstandard1.6 and that package was absorbed into netstandard2.0.

This is telling you that you made BaseNumberConverter effectively sealed when compared to the previous version from the package. That's intentional, as you mentioned.

Closer i look this isn't actually a new check, probably this just missed being checked in with #1176 and didn't fail CI.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wtgodbe I don't understand exactly why this wouldn't have failed CI. Maybe you're auto-baselining. That's fine to do for local builds, but maybe you should turn it off on CI.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, so these changes are expected and we just didn't baseline those before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericstj good idea - this should be a quick fix, I'll whip something up now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might not have time today, I'll circle back tomorrow morning - #1184

@terrajobst
Copy link
Member Author

I'm merging because CI is down. I verified that it builds though :-)

@terrajobst terrajobst merged commit a7b363d into dotnet:master May 9, 2019
@terrajobst terrajobst deleted the observable-bulk branch May 9, 2019 21:46
@hez2010
Copy link

hez2010 commented May 6, 2020

Hello. How can we use these bulk operations in .NET Standard 2.1? I didn't see such APIs exist in .NET Standard 2.1

@terrajobst
Copy link
Member Author

We had to revert this addition. See: #1222

@hez2010
Copy link

hez2010 commented May 7, 2020

Pity... Will it be added in the future?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
netstandard-api This tracks requests for standardizing APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collection<T> and ObservableCollection<T> do not support ranges