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

Call NLogPackageLoader.Preload (static) for NLog packages on load #1917

Merged
merged 1 commit into from
Mar 9, 2017

Conversation

304NotModified
Copy link
Member

@304NotModified 304NotModified commented Jan 14, 2017

This change is Reviewable

related: #1530

@codecov-io
Copy link

codecov-io commented Jan 14, 2017

Codecov Report

Merging #1917 into master will increase coverage by <1%.
The diff coverage is 86%.

@@           Coverage Diff           @@
##           master   #1917    +/-   ##
=======================================
+ Coverage      82%     82%   +<1%     
=======================================
  Files         285     285            
  Lines       19592   19628    +36     
  Branches     2290    2293     +3     
=======================================
+ Hits        15980   16021    +41     
+ Misses       3060    3040    -20     
- Partials      552     567    +15

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fdf100...f954491. Read the comment docs.

InternalLogger.Warn(e,"Preload invoked for '{0}' failed", type.FullName);
}

InternalLogger.Debug("Preload succesfully invoked for '{0}'", type.FullName);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line will be executed, even if invoke has been failed. I suggest that move this line to the line 267 (after the invoke).

}
else
{
InternalLogger.Debug("NLogPackageLoader preload method, but isn't static");
Copy link
Contributor

Choose a reason for hiding this comment

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

Log message would be improved as:

NLogPackageLoader contains a preload method, but it isn't static.

}

/// <summary>
/// Call Preload for <paramref name="type"/>. The method should be static.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be improved as:

Calls the preload method for <paramref name="type"/>. The preload method should be static.

@304NotModified
Copy link
Member Author

@UgurAldanmaz thanks for the review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants