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

Updated StringArgumentValue.ConvertTo to let the class not found exce… #67

Merged
merged 1 commit into from
Sep 7, 2017

Conversation

formix
Copy link

@formix formix commented Sep 6, 2017

…ption go

I lost a full afternoon looking for documentation or on stackoverflow to find out how to set an object instance to a method's "Args" in my appsettings.json file (same thing applies to the web.config I guess).

If I had recieved the correct exception, I would have known right away how to format the class name by looking at the stack trace and into the Type.GetType documentation.

I ran all unit tests and everything works fine with that change.

Hope it helps and you'll accept my pull request...

See my stackoverflow question here for some more background.

@nblumhardt
Copy link
Member

Hi Jean Philippe, thanks for the PR!

I think you're right- looking at the other places in Serilog.Settings.Configuration where we throw, it seems roughly in line with the rest of the library's behavior.

@serilog/reviewers-configuration any thoughts?

@skomis-mm
Copy link
Contributor

skomis-mm commented Sep 7, 2017

Looks great 👍 Good step to bring strictValidation feature eventially

@nblumhardt nblumhardt merged commit fae623c into serilog:dev Sep 7, 2017
@formix
Copy link
Author

formix commented Sep 7, 2017

W00t! My first accepted & merged pull request ever. I'll brag about that with my buddies. Just don't tell anyone that I changed just one line... shhhhhht.

Thanks and have a good one ;).

@khellang
Copy link
Member

khellang commented Sep 7, 2017

Congrats, @formix! 🎉 🍰 🍾

@nblumhardt nblumhardt mentioned this pull request Mar 14, 2018
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