-
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 config file loading: use process name (e.g. applicationname.exe.nlog) when app.config is not available #3261
Conversation
f1d2f29
to
2cd7e4a
Compare
Codecov Report
@@ Coverage Diff @@
## dev #3261 +/- ##
======================================
+ Coverage 80% 80% +<1%
======================================
Files 356 356
Lines 28007 28016 +9
Branches 3728 3728
======================================
+ Hits 22408 22438 +30
+ Misses 4520 4501 -19
+ Partials 1079 1077 -2 |
0bec3dc
to
fb51337
Compare
src/NLog/Internal/ProcessIDHelper.cs
Outdated
/// <summary> | ||
/// Initializes the ThreadIDHelper class. | ||
/// </summary> | ||
private static ProcessIDHelper CreateInstance() |
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.
👍 much better.
FYI: I think "instance" in redundant, and most "factory methods" are called Create - so renamed it
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.
Think the whole instance thing should be removed. But not sure about the reason for the Win32-native-call. Since the values are cached, then I don't understand remark about Win32 is "without the overhead".
Actually think that
Without any caching. Causing it to be much slower than this code:
But later caching was added so there no overhead at all for either method. Could be nice to remove some of those Win32-native-methods. |
Agree on that! Less code is better |
e5fe536
to
30c8d63
Compare
@snakefoot Have squashed the commits, and tried to ensure correct rename operations of the files. |
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.
Thanks for the changes!
I've a bit s problem with the no-so-lean interface. I know it's only internal, but still it's a degrade of the current interface (IFile). IMO it should be multiple interfaces which has clear separations (Single responsibility principle).
It looks like a lot of code has been moved? It's that really preferred? (Makes review and history more difficult too track)
f633c0e
to
d557d53
Compare
maybe we should update the docs - and this unfortunately related: #959 |
Well this PR actually tries to ensure that NetCore follows the Wiki :) |
so supersedes #959? Isn't this a breaking behavior change? |
4c64ca1
to
d0e19d6
Compare
No and No. #959 actually implements the config loading so it happens in the correct order. It should always use the application-specific process.exe.nlog instead of a random Nlog.config. But this is a breaking change, so you have stalled this fix for NLog 5.0 This PR only fixes NetCore so it behaves like NetFramework. Because |
Ok thanks for explaining! FYI, I like to start with merging 5.0 PRs after 4.6.3 is done. |
Think we should keep dev and master available for more hotfixes a few more weeks before starting up on NLog 5.0. But your decision. |
Hotfixes could be merged to master then :) (But features will be 5.0 instead of 4.6.*) |
@304NotModified Is the merge of this PR blocked by something? |
Yes my free time. I try to do some developing next to reviewing. Only reviewing is boring and I'm losing some touch to NLog in that way. |
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.
Thanks for the changes!
/// </summary> | ||
internal interface IFile | ||
internal interface IFileSystem |
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.
FYI, this was not the direction I would take. I think it's better to have 10 small interfaces then 3 medium.
Also, with the introduction of NSubstitute, really small interfaces are nice.
But as it's internal I think this is more than good enough 👍
Anyway, pushing has some results ;) |
Very understandable. I also prefer developing over reviewing :) Will try and reduce the number of PRs for the NLog-project, so everything is not about my PRs :) But thank you for the merge. Now there is some cleanup to do:
I don't mind fixing these two, unless they should be up-for-grabs? |
Trying to resolve #3156 for NetCore