-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Expose and test (I)Dictionary.GetValueOrDefault #16605
Conversation
} | ||
|
||
TValue value; | ||
if(dictionary.TryGetValue(key, out value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: space after opening paren
return value; | ||
} | ||
|
||
return defaultValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd personally have written this as
return dictionary.TryGetValue(key, out value) ? value : defaultValue;
but what you have is fine.
return value; | ||
} | ||
|
||
return defaultValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same nits
Assert.Throws<ArgumentNullException>("dictionary", () => dictionary.GetValueOrDefault("key", "value")); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: It'd be good to test with some other concrete type beyond Dictionary<TKey,TValue>
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, used SortedDictionary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @hughbe. LGTM once CI is green.
Addressed all your feedback, thanks! |
@@ -0,0 +1,4 @@ | |||
Compat issues with assembly System.Collections: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be a universal ApiCompatBaseline.txt. It should be specific to just uap (ApiCompatBaseline.uapaot.txt)
Thank you for your fix @hughbe! Really appreciate your help. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add APICompatBaseline
@karelz did this method make it to 4.7.2? Looks like it should, but MSDN says otherwise |
It was actually reverted back - see PR #17917. |
I see. Thanks for help |
* Expose and test (I)Dictionary.GetValueOrDefault * Address PR feedback Add APICompatBaseline Commit migrated from dotnet/corefx@d622dd4
Running
msbuild src\System.Collections\src\System.Collections.csproj /p:TargetGroup=uapaot /p:BaselineAllApiCompatError=true
doesn't seem to change anything but I hope this passes CIFixes #3482