Add TryAdd/Remove extension methods in CollectionExtensions by nbarbettini · Pull Request #18091 · dotnet/corefx

nbarbettini · 2017-04-07T20:52:16Z

Adding TryAdd and Remove to CollectionExtensions.

Hopefully I did everything correctly. If not, I'm happy to revise the PR 😄

Closes #17918

dnfclas · 2017-04-07T20:52:20Z

@nbarbettini,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

nbarbettini · 2017-04-07T20:55:14Z

-
-namespace System.Collections.Generic
-{
-    internal static class DictionaryExtensions


I think this is superseded by adding TryAdd to CollectionExtensions?

It's actually still needed. Dictionary.TryAdd only exists in .NET Core, not in desktop (.NET Framework), so this extension method is used by the tests when compiled for desktop to provide a polyfill.

Actually we would need to add the Remove API that is being added to CollectionExtensions in order for all the tests to pass in Desktop right, @stephentoub ?

Yup. Or alternatively include the product CollectionExtensions file into the test project when building for desktop, if that's feasible.

The reason I removed this is because was an ambiguous call with the new Collection.ExtensionsTryAdd. I'll investigate resolving the problem re: desktop.

Maybe you could wrap this type around an #if !netcoreapp

nbarbettini · 2017-04-07T20:56:11Z

 TypesMustExist : Type 'System.Data.SqlClient.SqlColumnEncryptionCngProvider' does not exist in the implementation but it does exist in the contract.
 TypesMustExist : Type 'System.Data.SqlClient.SqlColumnEncryptionCspProvider' does not exist in the implementation but it does exist in the contract.
 TypesMustExist : Type 'System.Data.SqlClient.SqlColumnEncryptionKeyStoreProvider' does not exist in the implementation but it does exist in the contract.
-CannotRemoveBaseTypeOrInterface : Type 'System.Data.SqlClient.SqlCommand' does not implement interface 'System.ICloneable' in the implementation but it does in the contract.


Not really sure why these showed up - I didn't change anything in System.Data.SqlClient. Should these be ignored?

This is created/updated automatically when running build.cmd and getting a successful build. This is a diff in APIs we have in between netcoreapp and desktop. So don't worry about this. You can either remove this change from your PR or leave it, sooner or later it will get updated.

nbarbettini · 2017-04-07T21:01:00Z

FYI, I can squash some of these commits if you'd like. I had a little bit of churn while I was figuring out how everything worked.

dnfclas · 2017-04-07T21:01:41Z

@nbarbettini, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

safern · 2017-04-07T21:35:30Z

Thanks for doing this!

I think deleting DictionaryExtensions.cs broke some other test projects that used this file that is why you are getting a lot of CI failures. To get a local repro an fix it you will have to build the tests locally for every target framework:

Netcoreapp:

From your repo root run the following commands:

build.cmd

build-tests.cmd -skiptests

Netfx:

From your repo root run the following commands:

build.cmd -framework:netfx

build-tests.cmd -skiptests -framework:netfx

Uap:

From your repo root run the following commands:

build.cmd -framework:uap

build-tests.cmd -skiptests -framework:uap

UapAot:

From your repo root run the following commands:

build.cmd -framework:uapaot

build-tests.cmd -skiptests -framework:uapaot

[EDIT]: @stephentoub is right we need that file for the tests in Desktop to pass.

stephentoub · 2017-04-07T22:57:19Z

+            }
+
+            return false;
+        }


This isn't ever removing the value from the dictionary.

(which also highlights that the new tests are missing verification that the value is actually removed)

Excellent point... d'oh. Fixed. 👍

safern · 2017-04-07T23:07:55Z

To run the tests in Desktop and verify that the new tests that you added are passing you can run from your repo's root:

build.cmd -framework:netfx
msbuild src\System.Collections\tests\System.Collections.Tests.csproj /t:rebuildandtest /p:TargetGroup=netfx

safern · 2017-04-08T01:38:25Z

      <Link>Common\System\ObjectCloner.cs</Link>
    </Compile>
-    <Compile Include="$(CommonTestPath)\System\Collections\DictionaryExtensions.cs">
-      <Link>Common\System\Collections\DictionaryExtensions.cs</Link>


There is around 7 other projects that use DictionaryExtensions.cs file.

That is why CI is failing. You could verify this by running:
build.cmd
build-tests.cmd -skiptests

Can we leave this file as it was and add the new API there as well? Since there is a lot of test projects that use this file.

hughbe · 2017-04-08T02:10:29Z

+        public void TryAdd_KeyDoesntExistInIDictionary_ReturnsTrue()
+        {
+            IDictionary<string, string> dictionary = new SortedDictionary<string, string>();
+            Assert.Equal(true, dictionary.TryAdd("key", "value"));


Nit: Assert.True or Assert.False instead of direct equality checks for booleans

stephentoub · 2017-04-08T02:38:04Z

+                throw new ArgumentNullException(nameof(dictionary));
+            }
+
+            value = default(TValue);


This should be moved down to where false is returned.

stephentoub · 2017-04-08T02:38:23Z

+
+            value = default(TValue);
+
+            if (dictionary.TryGetValue(key, out var foundValue))


Why use a temporary? You can just use out value.

nbarbettini · 2017-04-09T01:45:21Z

Thanks for the great feedback (and patience) y'all. Had to step out for a bit and didn't get back to it until now.

I reverted my deletion of DictionaryExtensions because I realize that's the wrong path to go down. There are some tests still failing because of an ambiguous call between the TryAdd in DictionaryExtensions and my new TryAdd. (I'm sure it's something simple, I'll keep working on it)

Thanks for sticking with me 😄

safern · 2017-04-11T01:51:28Z

Thanks for the update... please let us know if you are not able to figure it out so that we can guide you.

danmoseley · 2017-04-16T05:34:36Z

@dotnet-bot test this please (logs gone)

nbarbettini · 2017-04-16T20:31:58Z

Sorry for the delay, it's been a busy week!

Thanks for the hints @safern and @stephentoub. I was able to get all the tests to pass by selectively excluding the DictionaryExtensions file in the targets that don't need the shim. It could also be done with an #if !netcoreapp conditional in the file itself; is that preferable, or is this way preferable?

Let me know how it's looking, and if there's anything else I need to do. I can rebase the PR, and/or squash some of the churn in the history if you want.

safern · 2017-04-16T20:49:50Z

Sorry for the delay, it's been a busy week!

No worries!

Actually we rather do it the way you did it. I gave you another option in case you got blocked, but the way you did it is excellent. Thanks for the update.

The other thing you need to do now is fix your merge conflicts by fetching the latest changes in dotnet/master and rebasing your changes on top of it. Then update your PR. See: #18109 (comment) as a guidance. To update the PR after doing that you will have to do a force push.

nbarbettini · 2017-04-16T21:02:05Z

@safern Rebased and resolved conflicts. 👍

safern · 2017-04-16T21:02:52Z

+        public void Remove_NullKeyIDictionary_ThrowsArgumentNullException()
+        {
+            IDictionary<string, string> dictionary = new SortedDictionary<string, string>();
+            Assert.Throws<ArgumentNullException>("key", () => dictionary.Remove(null, out var value));


Good idea, done. 👍

safern · 2017-04-16T21:03:29Z

+        public void Remove_NullIDictionary_ThrowsArgumentNullException()
+        {
+            IDictionary<string, string> dictionary = null;
+            Assert.Throws<ArgumentNullException>("dictionary", () => dictionary.Remove("key", out var value));


We could assign a value to the out parameter before calling Remove and test that the value did not changed when an exception is thrown.

safern

Thanks @nbarbettini for contributing. Feel free to grab any issue with milestone 2.0.0. When grabbing the issue just ping on it so that we can assign it to you and we don't have two people working on the same issue.

nbarbettini · 2017-04-16T22:22:05Z

Thanks @safern et al! 🎉 I'm really glad to be able to contribute.

…8091) * Add TryAdd and Remove to CollectionExtensions

dnfclas added the cla-required label Apr 7, 2017

nbarbettini commented Apr 7, 2017

View reviewed changes

dnfclas added cla-signed and removed cla-required labels Apr 7, 2017

safern assigned safern, ianhays and nbarbettini Apr 7, 2017

safern added the area-System.Collections label Apr 7, 2017

safern added this to the 2.0.0 milestone Apr 7, 2017

stephentoub reviewed Apr 7, 2017

View reviewed changes

safern reviewed Apr 8, 2017

View reviewed changes

hughbe reviewed Apr 8, 2017

View reviewed changes

stephentoub reviewed Apr 8, 2017

View reviewed changes

nbarbettini added 5 commits April 16, 2017 14:00

Add TryAdd and Remove to CollectionExtensions

43aa376

Better implementation of Remove

1d0ef3c

Add tests

b6433f6

Fix bugs and finish tests

20f23b8

Remove references to DictionaryExtensions

2e1c830

nbarbettini added 6 commits April 16, 2017 14:00

Remove should actually remove items

cdccbbb

Only include on netfx builds

4f2a7f1

Clean up tests

57bad2f

Improve per PR feedback

34d27d8

Revert changes

b6f9115

Omit DictionaryExtensions from netcoreapp and uat

e5f005b

nbarbettini force-pushed the 17918-AddDictionaryAddRemoveExtensions branch from d6e7a33 to e5f005b Compare April 16, 2017 21:01

safern reviewed Apr 16, 2017

View reviewed changes

Harden Remove tests per PR feedback

02cfc7f

safern approved these changes Apr 16, 2017

View reviewed changes

safern merged commit c8ed5b6 into dotnet:master Apr 16, 2017

nbarbettini deleted the 17918-AddDictionaryAddRemoveExtensions branch April 16, 2017 22:58

WinCPP pushed a commit to WinCPP/corefx that referenced this pull request Apr 18, 2017

Add TryAdd/Remove extension methods in CollectionExtensions (dotnet#1…

b27d6fc

…8091) * Add TryAdd and Remove to CollectionExtensions

WinCPP pushed a commit to WinCPP/corefx that referenced this pull request Apr 18, 2017

Add TryAdd/Remove extension methods in CollectionExtensions (dotnet#1…

8d23a74

…8091) * Add TryAdd and Remove to CollectionExtensions


		value = default(TValue);

		if (dictionary.TryGetValue(key, out var foundValue))

Conversation

nbarbettini commented Apr 7, 2017

Uh oh!

dnfclas commented Apr 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safern Apr 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbarbettini commented Apr 7, 2017

Uh oh!

dnfclas commented Apr 7, 2017

Uh oh!

safern commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Netcoreapp:

Netfx:

Uap:

UapAot:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safern commented Apr 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbarbettini commented Apr 9, 2017

Uh oh!

safern commented Apr 11, 2017

Uh oh!

danmoseley commented Apr 16, 2017

Uh oh!

nbarbettini commented Apr 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

safern commented Apr 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nbarbettini commented Apr 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safern left a comment

Choose a reason for hiding this comment

Uh oh!

nbarbettini commented Apr 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

safern Apr 8, 2017 •

edited

Loading

safern commented Apr 7, 2017 •

edited

Loading

nbarbettini commented Apr 16, 2017 •

edited

Loading

safern commented Apr 16, 2017 •

edited

Loading