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.

Sign up for GitHub

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

Jump to bottom

XmlLoggingConfiguration: Set config values on correct LogFactory object #1225

Merged
merged 7 commits into from
Feb 20, 2016
Merged

XmlLoggingConfiguration: Set config values on correct LogFactory object #1225

merged 7 commits into from
Feb 20, 2016

Conversation

bhaeussermann
Copy link
Contributor

Related to #1203

@bhaeussermann bhaeussermann added bug Bug report / Bug fix has unittests labels Feb 7, 2016
@codecov-io
Copy link

Current coverage is 74.65%

Merging #1225 into master will increase coverage by +0.19% as of 2b92a5d

@@            master   #1225   diff @@
======================================
  Files          267     267       
  Stmts        15392   15407    +15
  Branches      1614    1615     +1
  Methods          0       0       
======================================
+ Hit          11461   11502    +41
+ Partial        381     380     -1
+ Missed        3550    3525    -25

Review entire Coverage Diff as of 2b92a5d

Uncovered Suggestions

  1. +0.09% via ...gingConfiguration.cs#282...295
  2. +0.09% via ...gingConfiguration.cs#248...261
  3. +0.08% via ...argets/FileTarget.cs#1704...1716
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

304NotModified
304NotModified reviewed Feb 7, 2016
View reviewed changes
src/NLog/Config/XmlLoggingConfiguration.cs
@@ -472,12 +476,18 @@ private void ParseNLogElement(NLogXmlElement nlogElement, string filePath, bool
if (filePath != null)
this.fileMustAutoReloadLookup[GetFileLookupKey(filePath)] = autoReload;

LogManager.ThrowExceptions = nlogElement.GetOptionalBooleanAttribute("throwExceptions", LogManager.ThrowExceptions);
if (customLogFactory == null)
Copy link
Member

Choose a reason for hiding this comment

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

maybe we can pass LogManager as custom factory? (maybe LogManager.Factory as internal readable?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

@304NotModified
Copy link
Member

Looks good. Added some notes. Binary compat is the most important one.

@bhaeussermann
Copy link
Contributor Author

AppVeyor:

LogManager.cs(137,24): error CS1574: Warning as Error: XML comment on 'NLog.LogManager.Configuration' has cref attribute 'LogFactory.Configuration' that could not be resolved [C:\projects\nlog\src\NLog\NLog.Xamarin.Android.csproj]

Wha?! I didn't touch this cref attribute and the LogFactory.Configuration property was not moved or renamed. Any ideas?

@304NotModified
Copy link
Member

Wha?! I didn't touch this cref attribute and the LogFactory.Configuration property was not moved or renamed. Any ideas?

A using has been removed?

It can we also a wrong auto-merge. Had that also once.

@bhaeussermann
Copy link
Contributor Author

@304NotModified I cannot see what is wrong. If I right-click on any of these crefs in Visual Studio and select "Go To Definition", it takes me to the corresponding member. So they are still correct according to Visual Studio.

@304NotModified
Copy link
Member

well I would recommend to merge with master locally and then check it.

@bhaeussermann
Copy link
Contributor Author

well I would recommend to merge with master locally and then check it.

The crefs still seem fine according to Visual Studio. I cannot see what changes in this PR might cause the crefs to break.

@304NotModified
Copy link
Member

Will check it locally tomorrow. Strange issue. Have to be a missing using or something like that.

@304NotModified
Copy link
Member

merge bhaeussermann#1 and it is fixed :)

some ambiguity about Conguration (class name vs property)

@bhaeussermann bhaeussermann removed their assignment Feb 20, 2016
@304NotModified 304NotModified added this to the 4.3 milestone Feb 20, 2016
304NotModified added a commit that referenced this pull request Feb 20, 2016
@304NotModified
Merge pull request #1225 from bhaeussermann/branch0
4cc3938 
XmlLoggingConfiguration: Set config values on correct LogFactory object
@304NotModified 304NotModified merged commit 4cc3938 into NLog:master Feb 20, 2016
@304NotModified
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@304NotModified 304NotModified

Labels
bug Bug report / Bug fix
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bhaeussermann @codecov-io @304NotModified