Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add resource value validation for DefaultFormatter #588

Merged
merged 5 commits into from
Dec 2, 2016

Conversation

Chrissx
Copy link

@Chrissx Chrissx commented Oct 2, 2016

Added some resource value validation to make the missing resources errors more clear.

Connected to #551 .

var resourceString = Resources.GetResource(GetResourceKey(resourceKey), _culture);

if (string.IsNullOrEmpty(resourceString))
throw new ArgumentException(string.Format("The resource object with key '{0}' was not found", resourceKey), nameof(resourceKey));
Copy link
Contributor

@aloisdg aloisdg Oct 3, 2016

Choose a reason for hiding this comment

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

You can use string interpolation.

string.Format("The resource object with key '{0}' was not found", resourceKey)

->

$"The resource object with key '{resourceKey}' was not found"

var resourceString = Resources.GetResource(GetResourceKey(resourceKey, number), _culture);

if (string.IsNullOrEmpty(resourceString))
throw new ArgumentException(string.Format("The resource object with key '{0}' was not found", resourceKey), nameof(resourceKey));
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use string interpolation.

string.Format("The resource object with key '{0}' was not found", resourceKey)

->

$"The resource object with key '{resourceKey}' was not found"

Copy link
Author

Choose a reason for hiding this comment

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

I was not using string interpolation here because if the Stylecop configured to this projects, it can break because you can't configure culture for string interpolation. Fixed now :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I was not aware of this fact. The more you know 😄

Copy link
Author

Choose a reason for hiding this comment

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

Unhandled Exception: System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt

I don't think so that was me :D Should I re-trigger?

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked too and I dont think so, but I cant be sure.

Copy link
Author

Choose a reason for hiding this comment

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

And It's building :)

@Chrissx
Copy link
Author

Chrissx commented Oct 12, 2016

🕐

@aloisdg
Copy link
Contributor

aloisdg commented Oct 12, 2016

@Chrissx ?

@Chrissx
Copy link
Author

Chrissx commented Oct 12, 2016

Ah sorry this was just a ping. Build was succeed. Anything that I need to do?

@aloisdg
Copy link
Contributor

aloisdg commented Oct 12, 2016

You can ping @thedillonb and @onovotny from the original issue.

@clairernovotny
Copy link
Member

@aloisdg feel free to merge when you're satisfied

@aloisdg
Copy link
Contributor

aloisdg commented Oct 12, 2016

@onovotny I am not a collaborator 😉

@clairernovotny
Copy link
Member

@aloisdg that can be remedied. try again ;)

@aloisdg
Copy link
Contributor

aloisdg commented Oct 12, 2016

@Chrissx Can you add some tests to check the ArgumentException? Also, you can fill the return clause (better comments would be nice too, but it is still better than before :) ).

@aloisdg
Copy link
Contributor

aloisdg commented Oct 12, 2016

@onovotny Done! I will try to be worthy. 😄

@clairernovotny clairernovotny merged commit 07bcaca into Humanizr:dev Dec 2, 2016
@clairernovotny clairernovotny added this to the vNext milestone Dec 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants