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

Config: Implemented inheritance policy for autoReload in included config files #1116

Merged
merged 6 commits into from
Dec 29, 2015

Conversation

bhaeussermann
Copy link
Contributor

Fixes #668

@codecov-io
Copy link

Current coverage is 73.18%

Merging #1116 into master will decrease coverage by -0.01% as of 8830a3b

@@            master   #1116   diff @@
======================================
  Files          264     264       
  Stmts        14928   14930     +2
  Branches      1636    1637     +1
  Methods          0       0       
======================================
  Hit          10927   10927       
  Partial        419     419       
- Missed        3582    3584     +2

Review entire Coverage Diff as of 8830a3b


Uncovered Suggestions

  1. +0.09% via ...nternal/AspHelper.cs#206...218
  2. +0.09% via ...nternal/AspHelper.cs#188...200
  3. +0.09% via ...nternal/AspHelper.cs#170...182
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

/// Gets or sets a value indicating whether the configuration files
/// should be watched for changes and reloaded automatically when changed.
/// </summary>
public bool AutoReload { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Can you tell me why this property is not needed anymore? it's now a breaking change!

@304NotModified
Copy link
Member

Thanks! Which unit tests are new now?

This is now a breaking change. We should try to get it non-breaking.

@304NotModified 304NotModified added the breaking change Breaking API change (different to semantic change) label Dec 28, 2015
@bhaeussermann
Copy link
Contributor Author

Thanks! Which unit tests are new now?

Auto_Reload_after_rename() in LogFactoryTests has been replaced by TestAutoReloadOnFileCopy().
Auto_reload_validxml_test() and Auto_reload_invalidxml_test() have been replaced by TestAutoReloadOnFileChange().

All the other tests are new.

…fig files.

Added back AutoReload property to support clients who might use it.
…fig files.

Make all method parameters required.
@bhaeussermann
Copy link
Contributor Author

@304NotModified I have applied your feedback:

  • Added back the AutoReload property.
  • Made all method parameters mandatory.

@304NotModified
Copy link
Member

Thanks!

IMO arrays are legacy stuff (and evil). Can you replace toArray with toList? Then I will merge it :)

@304NotModified 304NotModified added bug Bug report / Bug fix and removed breaking change Breaking API change (different to semantic change) labels Dec 28, 2015
@304NotModified 304NotModified added this to the 4.3 milestone Dec 28, 2015
@bhaeussermann
Copy link
Contributor Author

IMO arrays are legacy stuff (and evil). Can you replace toArray with toList? Then I will merge it :)

Done ;).

@304NotModified
Copy link
Member

NICE!

304NotModified added a commit that referenced this pull request Dec 29, 2015
Config: Implemented inheritance policy for autoReload in included config files
@304NotModified 304NotModified merged commit 64fdb41 into NLog:master Dec 29, 2015
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.

3 participants