-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
NLog.LogManager.Setup() - Introduced LoadConfiguration and friends for ISetupBuilder #3871
NLog.LogManager.Setup() - Introduced LoadConfiguration and friends for ISetupBuilder #3871
Conversation
Notice no unit-tests yet, because I'm guessing we are entering discussion mode first. |
e019da5
to
80204a8
Compare
80204a8
to
36a65b6
Compare
36a65b6
to
501d5c2
Compare
e725aa2
to
0a04748
Compare
1c6b161
to
833b721
Compare
How does this works if we load from file and enhance it from API? (so it isn't a fallback). And if we do reload, does that work then?
I think it's nice! Only some naming things I guess |
Notice that the two last code-examples are a little different (fallback when no config):
And the other (enhancing loaded config):
Ofcourse with Like I said in my introduction of this. Then it is a very small dive into the fluent-setup of NLog-config. |
Created some unit-tests that increases code-coverage. |
378f07e
to
cb39c21
Compare
Btw. happy to see that NLog 4.7.0 seems to be swimming nicely along without any major issues. Not like NLog 4.6.0 where my refactoring of XmlLoggingConfiguration (To load NLog.config from appsettings.json) caused all kind of bad experiences. But maybe I have just jinxed it, and the radio-silence comes from the corona dance :) |
cb39c21
to
5ec96ae
Compare
Should we include a If you would like to remove the So this:
Becomes this:
|
Sounds good! |
Created PR #3909 |
One name thing, would it be easier? (So drop the NLog in the name)
|
I did that on purpose, because there is both "NLog.config" and "appsettings.json" and "app.config". All of them are files. NLog gives special love and attention to |
Ah well I expected that "appsettings.json" and "app.config" would work (in the future) 👼 What about |
Ow that won't scale nice between other packages. I stil prefer LoadConfig, but I think "appsettings.json" should be named other, e.g. LoadConfigFromAppsettingsJson |
Never going to happen, since all of them have different dependencies, so they would require different assemblies (and extension-methods). Just like you see is happening in NLog.Extension.Logging and Nlog.Web. P.S. It was my dream when I introduced |
src/NLog/SetupBuilderExtensions.cs
Outdated
/// <summary> | ||
/// Loads NLog config from filename <paramref name="configFile"/> if provided, else fallback to scanning for NLog.config | ||
/// </summary> | ||
public static ISetupBuilder LoadNLogConfigFromFile(this ISetupBuilder setupBuilder, string configFile = null, bool optional = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: configFilePath?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like one is only giving the candidate-path to the NLog.config
file. Not sure it is an improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well configFile
sounds a bit like the content of the configFile.
To what is this referring? |
My dream that LogManager.LoadConfiguration should load all kind of files. |
ow yes, that would be nice. But you still prefer LoadNLogConfig over LoadConfig? I never saw the relation between "NLog" word and NLog.config (as dumb user is still would try |
Yes to me LoadNLogConfig is better than LoadConfig. As it signals the file has to match the format of NLog.config. And the contents of the config is the NLog LoggingConfiguration, and not some other config. |
I guess these two methods could have different name:
Could also be called:
|
Changed builder-method from LogFactory to |
That's IMO better. I don't think "NLog Should be in a method name, as we're already in a NLog context. Why about LogManager.Setup().LoadConfiguration? It is consistent with |
426c07b
to
cfe7104
Compare
cfe7104
to
5f8f17f
Compare
Have updated the PR with the new names. Some adjustment is needed for: |
Kudos, SonarCloud Quality Gate passed! 0 Bugs |
Cool! Thanks! |
Very small dive into the idea of fluent-api for setup of NLog-LoggingConfiguration:
This will match the PRs here:
NLog/NLog.Extensions.Logging#418
NLog/NLog.Web#540
This allows one to setup a fallback configuration, if no
NLog.config
file was found:Or one can so this (Resolves almost #1429, but without
autoReload=true
support)One can also setup support for loading NLog.config first in Process-Directory (Before NLog5 is released)