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

LoggingConfiguration - Rollback static Reload-method #3335

Merged

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Apr 26, 2019

Rollback of #3306 until a better solution comes around for NLog/NLog.Extensions.Logging#283

@snakefoot snakefoot force-pushed the LoggingConfigurationReloadAsActive branch from a3f481d to ed0109e Compare April 26, 2019 18:24
@codecov-io
Copy link

codecov-io commented Apr 26, 2019

Codecov Report

Merging #3335 into dev will increase coverage by <1%.
The diff coverage is 42%.

@@          Coverage Diff           @@
##             dev   #3335    +/-   ##
======================================
+ Coverage     80%     80%   +<1%     
======================================
  Files        356     356            
  Lines      28226   28222     -4     
  Branches    3763    3762     -1     
======================================
+ Hits       22579   22586     +7     
+ Misses      4558    4549     -9     
+ Partials    1089    1087     -2

@304NotModified
Copy link
Member

Hi, thanks for the PR,

but would it be more logical to have something like this:

LogManager.ReloadConfiguration();
// and/or
LogFactory.ReloadConfiguration();

I find the "active" a bit unusual

@snakefoot
Copy link
Contributor Author

snakefoot commented Apr 26, 2019

I find the "active" a bit unusual

I need a method on LoggingConfiguraion (or method that takes LoggingConfiguration as parameter), that allows one to reload the config with respect to KeepVariablesOnReload (For use in NLog.Extension.Logging).

But also thought it would be nice to provide a solution for #3304

But maybe I should just remove the new introduced method completely. Since I can probably implement this variable copy manually in NLog.Extension.Logging.

@304NotModified
Copy link
Member

But maybe I should just remove the new introduced method completely. Since I can probably implement this variable copy manually in NLog.Extension.Logging.

I'm also doubting this addition makes things easier, so for now I think

 NLog.LogManager.Configuration = NLog.LogManager.Configuration.Reload();

Is good enough

@snakefoot
Copy link
Contributor Author

Is good enough

But it will not support KeepVariablesOnReload

@304NotModified
Copy link
Member

but would it be more logical to have something like this:

LogManager.ReloadConfiguration();
// and/or
LogFactory.ReloadConfiguration();

Can't be do newConfig.CopyVariables(currentConfig.Variables); in e.g. the LogManager to arrange this?

@snakefoot snakefoot force-pushed the LoggingConfigurationReloadAsActive branch from ed0109e to 7e54a45 Compare April 27, 2019 11:52
@snakefoot
Copy link
Contributor Author

in e.g. the LogManager to arrange this?

Sounds like something for NLog 5.0. Lets just rollback #3306

@snakefoot snakefoot changed the title LoggingConfiguration - ReloadAsActive() with KeepVariablesOnReload support LoggingConfiguration - Rollback adding static Reload Apr 27, 2019
@snakefoot snakefoot changed the title LoggingConfiguration - Rollback adding static Reload LoggingConfiguration - Rollback adding static Reload-method Apr 27, 2019
@snakefoot snakefoot changed the title LoggingConfiguration - Rollback adding static Reload-method LoggingConfiguration - Rollback static Reload-method Apr 27, 2019
@304NotModified
Copy link
Member

I'm confused. You forced pushed different content? Won't it be more logical to close the old PR and create a new one?

@snakefoot
Copy link
Contributor Author

Hate to waste a precious PulRequest-number :)

@304NotModified 304NotModified added this to the 4.6.3 milestone Apr 28, 2019
@304NotModified
Copy link
Member

This is still valid, isn't?

@snakefoot
Copy link
Contributor Author

snakefoot commented Apr 28, 2019 via email

@304NotModified 304NotModified added the enhancement Improvement on existing feature label Apr 28, 2019
@304NotModified 304NotModified merged commit ebb6d2b into NLog:dev Apr 28, 2019
@snakefoot snakefoot deleted the LoggingConfigurationReloadAsActive branch April 4, 2020 12:57
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