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

Throw exception when base.InitializeTarget() is not called + inline GetAllLayouts() #1191

Merged
merged 6 commits into from
Feb 2, 2016

Conversation

304NotModified
Copy link
Member

  • Target.GetAllLayouts() was useless a method (once called, private). So inlined it.
  • If you write a target and forgot to call base.InitializeTarget(), you can get a nullRefException in Target.PrecalculateVolatileLayouts (called in AsyncWrapper/BufferWrapper). The nullref is now fixed, but you still get odd behaviour, so we will check ifbase.InitializeTarget()` has been called.

@codecov-io
Copy link

Current coverage is 74.03%

Merging #1191 into master will increase coverage by +0.03% as of 145b1f8

@@            master   #1191   diff @@
======================================
  Files          265     265       
  Stmts        15296   15299     +3
  Branches      1679    1681     +2
  Methods          0       0       
======================================
+ Hit          11320   11326     +6
- Partial        417     419     +2
+ Missed        3559    3554     -5

Review entire Coverage Diff as of 145b1f8


Uncovered Suggestions

  1. +0.08% via ...argets/FileTarget.cs#1722...1734
  2. +0.07% via ...gingConfiguration.cs#282...293
  3. +0.07% via ...gingConfiguration.cs#244...255
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

@bhaeussermann
Copy link
Contributor

Whoops! It seems after merging the test now throws NLogRuntimeException instead of NLogConfigurationException. The original exception is no longer caught and rethrown as an NLogConfigurationException. This is probably fine as the exception does not occur due to a configuration error. What do you think?

@304NotModified
Copy link
Member Author

Whoops! It seems after merging the test now throws NLogRuntimeException instead of NLogConfigurationException. The original exception is no longer caught and rethrown as an NLogConfigurationException. This is probably fine as the exception does not occur due to a configuration error. What do you think?

I think NLogRuntimeException is more suitable?

@304NotModified 304NotModified added the enhancement Improvement on existing feature label Feb 2, 2016
@304NotModified
Copy link
Member Author

Changed to NLogRuntimeException is the unit test

304NotModified added a commit that referenced this pull request Feb 2, 2016
Throw exception when base.InitializeTarget() is not called + inline GetAllLayouts()
@304NotModified 304NotModified merged commit 3c18412 into master Feb 2, 2016
@304NotModified 304NotModified deleted the InitializeTarget-is-required branch February 3, 2016 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement on existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants