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

NLogFactory - Removed explicit loading of nlog.config by default #419

Closed
wants to merge 3 commits into from

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Nov 11, 2018

Since NLog will automatically load its configuration. Resolves #418

See also: https://github.com/nlog/NLog/wiki/Configuration-file#configuration-file-locations

…ce NLog will automatically load its configuration
Copy link
Member

@stakx stakx left a comment

Choose a reason for hiding this comment

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

Since this changes the default behavior of NLogFactory, I think this change should be mentioned in the changelog.

@snakefoot
Copy link
Contributor Author

snakefoot commented Nov 12, 2018

@stakx Have now updated the ChangeLog.md

Copy link
Member

@stakx stakx left a comment

Choose a reason for hiding this comment

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

I am not overly familiar with NLog and Castle's logging facilities, but your change looks good to me in principle. Here's a couple of thoughts I had while looking over your PR.

CHANGELOG.md Outdated Show resolved Hide resolved
…ult, since NLog will automatically load its configuration
@devop228
Copy link

I look at the constructor overload NLogFactory(bool configuredExternally), its existence seems redundant since it leads to almost same behavior on both path of the code. Would removing it completely be too much a breaking change?

@snakefoot
Copy link
Contributor Author

snakefoot commented Nov 13, 2018 via email

@stakx stakx mentioned this pull request Dec 9, 2018
@jonorossi
Copy link
Member

I look at the constructor overload NLogFactory(bool configuredExternally), its existence seems redundant since it leads to almost same behavior on both path of the code. Would removing it completely be too much a breaking change?

@devop228 that is what I proposed in #418 before this pull request was created.

Since Windsor's logging facility only has an ConfiguredExternally method, you cannot get Windsor to pass false unless you use the custom factory overload.

I'm going to close this pull request because I think just doing this makes thing inconsistent between log4net and NLog, and doesn't push us to where we want to be with the factory's being just a shim, and not setting up configuration.

@jonorossi jonorossi closed this Dec 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants