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

Fix #283: throwExceptions ="false" but Is still an error #465

Merged
3 commits merged into from
Dec 29, 2014

Conversation

YuLad
Copy link
Contributor

@YuLad YuLad commented Dec 11, 2014

Altered conditions in the MustBeRethrown extension method.
Fixed relevant tests and added a new one.


I'm unsure of what to do with the ConditionParseException:

  • should it be thrown even if throwException="false"?
  • should it be sub-classed from NLogConfigurationException and swallowed as well?

Altered conditions in the MustBeRethrown extension method.

Fixed relevant tests and added a new one.
@ilya-g
Copy link
Contributor

ilya-g commented Dec 12, 2014

Are you sure NLogConfigurationException should be suppressed when throwExceptions is false? It goes against NLog Exception Handling Rules introduced in v2.0.
If you are looking the way to fix issue #403 (application crashes on configuration reload if it encounters config error), the better approach would be to handle NLogConfigurationError only when reloading configuration, not on initial load.

@YuLad
Copy link
Contributor Author

YuLad commented Dec 13, 2014

@ilya-g I didn't encounter the exception handling rules while going through the wiki, thanks for the pointers. I think the latest commit should fix this issue, but is there anything else I might have missed?

@ghost ghost added this to the 4.0 milestone Dec 29, 2014
@ghost ghost added the bug Bug report / Bug fix label Dec 29, 2014
ghost pushed a commit that referenced this pull request Dec 29, 2014
Fix #283: throwExceptions ="false" but Is still an error
@ghost ghost merged commit 9f0a2c1 into NLog:master Dec 29, 2014
@YuLad YuLad deleted the issue-283 branch December 29, 2014 18:52
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report / Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants