Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Key item added to KeyNotFoundException#15201

Merged
danmoseley merged 1 commit into
dotnet:masterfrom
Anipik:keynotfound
Nov 24, 2017
Merged

Key item added to KeyNotFoundException#15201
danmoseley merged 1 commit into
dotnet:masterfrom
Anipik:keynotfound

Conversation

@Anipik
Copy link
Copy Markdown

@Anipik Anipik commented Nov 23, 2017

issue- dotnet/corefx#5188
corefx pr - dotnet/corefx#25472

@Anipik Anipik requested a review from danmoseley November 23, 2017 21:38
@Anipik Anipik changed the title Added Key in KeyNotFoundException Key item added to KeyNotFoundException Nov 23, 2017
@Anipik
Copy link
Copy Markdown
Author

Anipik commented Nov 24, 2017

@dotnet-bot test CentOS7.1 x64 Checked Innerloop Build and Test

@danmoseley
Copy link
Copy Markdown
Member

LGTM

@danmoseley danmoseley merged commit db342eb into dotnet:master Nov 24, 2017
dotnet-bot pushed a commit to dotnet/corert that referenced this pull request Nov 24, 2017
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Nov 24, 2017
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
internal static void ThrowKeyNotFoundException(object key)
{
throw new KeyNotFoundException();
throw new KeyNotFoundException(key.ToString());
Copy link
Copy Markdown
Member

@jkotas jkotas Nov 25, 2017

Choose a reason for hiding this comment

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

@Anipik This needs to use SR.Format(SR.Arg_KeyNotFoundWithKey, key.ToString()) to get the proper error message.

Also, this may need to be split into the Throw + Get...Exception method pair like the other similar methods in this file to allow JIT to generate better code for it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

isn't both throw + get already separated as we have ThrowKeyNotFoundException(object key) and KeyNotFoundException(SR.Format(SR.Arg_KeyNotFoundWithKey, key.ToString())); functions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Separating it even more has additional benefits - check the discussion in #7580 that introduced it.

@danmoseley
Copy link
Copy Markdown
Member

@brianrob I should have asked @Anipik to wait on your review. Is the change OK?

<value>The given key was not present in the dictionary.</value>
</data>
<data name="Arg_KeyNotFoundWithKey" xml:space="preserve">
<value>The given key '{0}' was not present in the dictionary.</value>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Indenting issue here.

jkotas pushed a commit to dotnet/corert that referenced this pull request Nov 25, 2017
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
danmoseley pushed a commit to dotnet/corefx that referenced this pull request Nov 25, 2017
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
@brianrob
Copy link
Copy Markdown
Member

@danmosemsft, this change is fine. However, it is going to break CoreFX and CoreRT because the use of the string is in the shared directory but the string definition is not. So, the string change needs to be ported to both corefx and corert.

@danmoseley
Copy link
Copy Markdown
Member

@Anipik note you have feedback above to address.

@Anipik
Copy link
Copy Markdown
Author

Anipik commented Nov 27, 2017

@brianrob dotnet/corefx#25472 contains the changes for the issue. Are they sufficient or we require to change anything else in corefx ?

@brianrob
Copy link
Copy Markdown
Member

@Anipik, corefx was fixed because the change came in via the mirror and @danmosemsft added 9a5f587 to it.

It looks like @jkotas fixed corert via da979a318e7d39498895365c60eb4a0c88d2a1d3 so I think you're good to go.

@Anipik Anipik deleted the keynotfound branch December 13, 2017 22:26
dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Jan 13, 2018
Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Jan 13, 2018
Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
safern pushed a commit to dotnet/corefx that referenced this pull request Jan 16, 2018
Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
safern pushed a commit to dotnet/corefx that referenced this pull request Jan 16, 2018
Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants